Deprecating ES6 compatibility layer


(Sam Saffron) #1

While profiling Discourse on my gigantic Nexus 6P which is the flagship Google phone at the moment I discovered we are spending a huge amount of time “requiring” various files on intial load.

Our boot sequence for the front page is pretty much this

  1. Download JS files and parse them
  2. Ember will “evaluates” itself requiring all its modules, this costs 290ms. Discussing options with @stefan_penner on speeding this up.
  3. Discourse invokes its “compatibility layer” evaluating every ES6 module we ship, this costs 700ms
  4. Ember runs various intializers (both internal to Ember and ones we wrote) 400ms
  5. Ember starts up the Ember router 140ms
  6. We turn our JSON into Emberish objects 300ms
  7. We render the front page … 1700ms a huge part of which is looking up factories in containers in Ember (150ms) and applying class name bindings (300ms)

We clearly want to tackle everything here, but a very easy win which in local testing saved us 580ms is removing the ES6 compatibility layer.

What is this compatibility layer?

Before @eviltrout moved us to ES6 all of Discourse’s JS was defined under the Discourse global.

Want a User class, use Discourse.User … want the UserNotificationsController use Discourse.UserNotificationsController.

This worked alright but had a very huge drawback, we were forced to evaluate large amounts of code we have no need to.

Evaluating classes in Ember can be VERY expensive when lots of classes are involved. The innocuous snippet below can cost 3ms.

var Something = RestModel.extend();

This is due to Ember’s object model and implementation of mixins, initilizers, overrides, computed properties and so on.

Ember does a lot of fiddeling with object prototypes, so the convenience of OO like features in a prototypel language does not come free.

We need to avoid all this work of defining our entire object model before rendering a single page and ES6 makes this very easy.

What will happen?

At the moment we have a “special” flag called DISCOURSE_NO_CONSTANTS which strips off the compatibility layer.

I will be working on getting everything in core and core plugins working in this mode. Once I feel this is ready:

  1. I will deploy meta without the compatibility layer
  2. A few days later I will deploy standard and business containers without the layer
  3. A few days later the flag will be removed and become default

How do you migrate to the new pattern?

We strongly recommend you use ES6 for all your plugin needs, when using ES6 you would do:

// old code
Discourse.PermissionType.create();

// new code
import PermissionType from 'discourse/models/permission_type';
PermissionType.create();

In some cases you may not be able to use ES6. (for example, site customizations or narrow backwards compat conditions)

var PermissionType = require('discourse/models/permission_type').default;
PermissionType.create();

How do I know what to require?

Usually guessing will do fine … Discourse.User maps to discourse/models/user … its in the models directory under user.

In rare cases you may need to browse: javascripts/discourse to find the actual file name.

Some stuff is going to bypass the need for a move

I will be whitelisting a few globals such as Discourse.Route and Discourse.Model, main reason is that they are always needed for any page and this cuts down on ceremony.

How long until this is all done?

I expect most of the changes to be done by the end of next week so expect the new default to land in master in 2 weeks or so

What can plugin authors do?

Be sure to run your plugin in with:

rm -fr /tmp/cache
DISCOURSE_NO_CONSTANTS=1 bin/thin start

This will ensure your plugin will be compatible when the new default lands.


Sam's personal "minimal" topic list design
Retort - a reaction-style plugin for Discourse
Cannot read property 'reopen' of undefined
CategoryTitleLinkComponent is undefined
showOpLikes - How to enable?
Persona Login Plugin
Linking to the same domain without Ember taking over
Likes columns gone after upgraded
Retort - a reaction-style plugin for Discourse
(Sam Saffron) #2

FYI I just committed a fairly big refactor to allow booting Discourse with DISCOURSE_NO_CONSTANTS, some cleanup is still needed but its getting very close

One notable process change that I think is worth mentioning.

I agree with using the form require('discourse/models/something').default over import in 3 conditions in ES6

  1. For narrow cases when you are trying to avoid circular dependencies

  2. For cases where the import is used ONLY once in the file, this helps keep the dependency graph shallow and helps defer dependency resolution till it is needed.

  3. In the interim, while have not moved to DISCOURSE_NO_CONSTANTS we can not depend on yet to be defined modules. In some cases to avoid “future dependencies” I am using require. It ends up leaner anyway so we win overall.


(cpradio) #3

I’m looking at testing all of my plugins with this constant over the next week or two, right now I use the following command to start rails on my development machine:

bundle exec rails s -b 0.0.0.0

I think I need to switch it to

export DISCOURSE_NO_CONSTANTS=1;
bundle exec rails s -b 0.0.0.0

I’m just trying to make sure I start the Discourse app with the constant being utilized, so I can verify my plugins operate with it.


(Sam Saffron) #4

Yes that should do the trick, you could add it to your .bashrc or .zshrc if you wanted … may switch it on by default in dev pretty soon.


(cpradio) #5

Cool, okay, so here is my new challenge.

This now stops the ability to detect older versions of Discourse to determine which composer button logic to run.

So the question becomes, is there a better way to detect the old composer? I’m going to play around with it a bit too, might not get very far today with it though.


(Sam Saffron) #6

Something along the lines of.

var newComposer;
try {
  newComposer = require('discourse/components/composer-editor').default;
} catch(e) {
  // you are running an old composer ...
}

(cpradio) #7

Awesome, I had to make a little adjustment to use discourse/components/d-editor though, as that is where the action needs to live for the toolbar button click to work. :smile:


(Robin Ward) #8

I disagree with this. You’re talking about one extra line of code in exchange for having to assume that the global exists everywhere. I much prefer explicitly including them. In the latest Ember CLI you have to include Ember even. I think we should follow their lead.

The goal should be to even import Discourse if we need it (note: most of the time we won’t!)

I would vastly prefer we fix the circular dependencies than have a mix of imports and require. If we allow people to do this, our code will quickly become littered with a mix of requires and imports which I absolutely do not want.

Additionally, it locks us in from using a different compiled format that doesn’t use require or a shim. We can remove many of the requireModule calls by compiling to a different target format if we want.

I’m okay with using it until we have migrated to modules everywhere as a workaround, though. But it should be considered a TODO to replace.


(Robin Ward) #9

Update: We had to revert this for now to do some emergency deploys. I think since it’s the weekend in Australia and about to be in Canada, we should put it back on Monday so we’ll be around to catch the many potential regressions.

All three commits were reverted here. If we revert this they’ll go back in:


(Sam Saffron) #10

I am ok with moving to require everything, it will have to wait till we move to the new no constants mode.

My big issue is the current implementation of the transpiler has 2 very big gaps I want to address.

  1. Each import forces a runtime full dependency resolution on first hit
  2. Import forces stuff to load unconditionally on start

Both of these can be resolved with some creativity.

Instead of running the dependency resolution at runtime we can ship a dependency graph, and delete stuff off the graph as it is loaded (which means we do not need to reify). I also want to get rid of the polymorphism on the callback function and use hidden globals instead.

So for example

// baz.js.es6
import Something from 'foo'; 
import foo from 'bar';

foo.hello();
bar.hello() 

// currently transpiles to roughly to a loader function that looks like this
function(deps) {
   var foo = deps._dep1_.default;
   var bar = deps._dep2_.default;

   foo.hello();
   bar.hello();
}

// instead I would like the loader function of this and have require be smart enough to preload dependencies into the global
function() {
   var foo = __deps__.foo.default;
   var bar = __deps__.bar.default;

   foo.hello();
   bar.hello();
}

This would get rid of a large chunk of work requiring tends to do, we could ship with a tree structure of dependencies:

foo = new Dep('foo');
bar = new Dep('bar');
baz = new Dep('baz');

baz.dependencies.push(bar);
baz.dependencies.push(foo);

// rough code for require baz

dep = DepMap['baz'];
if (dep) {
  // not loaded

  // recurse baz dependencies and load

  DepMap.delete 'baz'
}

Another optimisation I would like to add is support magic comment that tells us to “defer” load a dependency

// defer Foo
import Foo from 'foo';

function somethingThatAlmostNeverIsCalled() {
   foo.something();
}

// transpile to

function() {
   function somethingThatAlmostNeverIsCalled() {
     require('foo');
     __deps__.foo.something();
   }
}

With both of these changes I would be very comfortable moving to no globals for anything anywhere, I just don’t want us to get to a state similar to Ember where we are spending 100s of milliseconds in dependency resolution at runtime.

I also want to play with this kind of new pattern for building Ember itself.


(Sam Saffron) #11

DISCOURSE_NO_CONSTANTS is now running live on meta … this shaved 3 WHOLE SECONDS off initial load time on my Doogee X5

Let me know if you notice any bugs.


(Sam) #12

Supposing it’s related - getting some infinite/very-long reloads.

Repro:

  • Go to /latest
  • Click on the nav entry for latest (or the top-left logo link)
  • Also works on /categories, /new, /unread, etc, just use a link to the same page (e.g. /categories and the categories nav entry)

Can reproduce in Chrome & Firefox, Win7 Desktop.

Error console spits out TypeError: a.hideUniformCategory is not a function and/or TypeError: Cannot read property 'list' of undefined when this happens.


(Sam Saffron) #13

I fixed this issue a few hours ago, thanks for reporting!


(Robin Ward) #14

The ES6 module transpiler had a different format called bundle that did something very similar to this. I always wanted to move us to it. It looks like the es6-module-transpiler is deprecated (although we still use it) but it might be worth figuring out if this has been ported as a babel plugin or something similar.

I am okay with this if the Javascript works exactly the same with or without the hint. It’s really important to me that others who use our code can copy and paste it and not have to have the same transpiler in place to make it work.


(Joe Seyfried) #15

Pardon my stupid question, but: how do we import from user scripts in CSS/HTML customizations?

I keep getting WARNING: DEPRECATION: ´Discourse.PageTracker´ is deprecated, import the module. - and this gets me increasingly worried in the light of the upcoming changes.

I cannot use an

import PageTracker from "discourse/lib/page-tracker"; 

in a <script>, so what are the options here?


(Sam Saffron) #16

var PageTracker = require('discourse/lib/page-tracker').default;


(Jeff Atwood) #17

How much of this is improved in your recent commit that removed support for compiling views/templates (Ember and Handlebars) in production?


(Sam Saffron) #18

The biggest change is:

This now takes 120ms, but there are a bunch of other changes as well, no handlebars and ember template compiler (so less memory, less work) and so on.


(Sam Saffron) #19

@plugin_authors this is getting rather urgent now, be sure that your plugin works with DISCOURSE_NO_CONSTANTS=1 if it does not, it will be breaking very soon.


Babble - A Chat Plugin [ARCHIVE]
(cpradio) #21

Thanks @sam!

I believe I’ve got mine all setup. At least they work locally. Going to have to wait to deploy it to my test server later this week.