Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issue/2647 add es6-8 support #2648

Merged
merged 24 commits into from
Mar 19, 2020
Merged

issue/2647 add es6-8 support #2648

merged 24 commits into from
Mar 19, 2020

Conversation

oliverfoster
Copy link
Member

@oliverfoster oliverfoster commented Feb 17, 2020

fixes #2647

This pr allows us to use a large amount more ES6, ES7 and ES8, such as: classes to replace and extend from backbone ones, arrow functions, spread operators, async/await, Object.assign, Array.filter etc. all of which should work in IE11 given these changes.

Limitations

We're still using requirejs to bundle modules so we won't be able to use ES6 module import/export directives yet.
As requirejs uses Esprima to turn JavaScript into AST, the compilation process is limited to whatever Esprima can parse, it claims full support for ES8. This discounts some newer features of ECMAScript, such as ES9 object property spreading:

var object1 = { a: 1 };
var object2 = { b: 2, ...object1 };

requirejs will error at compile time with warnings for unsupported syntax, like so:
image

Function argument spreading works as expected.

function test(a, b, c) {
   debugger;
}
test(...[1, 2, 3])

Changes

  • created a compilation temp folder
    • to provide a space for interim asset production
  • changed requirejs javascript compiler
    • to output to temp folder
    • don't minify (would use uglify which isn't es6 compatible)
  • added babel transpiler task to grunt
    • take bundled javascript from temp folder
    • import and modify sourcemaps if available (so that the es6 sourcemaps map to minified es5 output)
    • transpile to es5
    • minify output
    • put output in outputdir/adapt/js/ as normal
  • added clean task to remove temp folder after compilation
  • updated backbone to v1.4.0
    • this adds a preinitialize function to model, view, collection, etc
    • updated backbone.controller to add preinitialize function
    • preinitialize allows es6 classes to setup events, defaults etc before the backbone initialize function calls
  • added polyfill for ie11
  • removed redundant polyfill for promises
  • merged AdaptModel ES6 PR issue/2647 Converted AdaptModel to ES Class #2652

Notes

I was thinking we could add this to version 5 to provide some backwards compatibility for plugins which will be updated in v6+.
If we convert backbone classes to es6 classes in the core (we definitely should do this), then we will need to define constructor properties as static getters:

static get type() {
  return 'component';
}

Testing

The PR #2651 has been accepted and merged into this one such that this PR contains client-side ES6-8 and updated build tools.

@oliverfoster oliverfoster changed the title issue/2647 add es6+ support issue/2647 add es6-8 support Feb 17, 2020
@chucklorenz
Copy link
Member

tested various (easy) features from ES6-ES8: arrow function, array.filter, Object.assign, Object.values, padStart, async/await. All successful.
👍

Copy link
Member

@jamesrea83 jamesrea83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

@guywillis
Copy link
Contributor

👀

@moloko
Copy link
Contributor

moloko commented Feb 21, 2020

can we hold off on merging this and #2652 until I've had a chance to do a release of what's currently in master (plus maybe one or two other things)?

@oliverfoster
Copy link
Member Author

of course, we still don't have the requisite +1s, so you should be safe regardless

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

@oliverfoster oliverfoster added this to the Adapt v6 milestone Mar 2, 2020
Copy link
Contributor

@tomgreenfield tomgreenfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting the following in the Chrome console:

DevTools failed to parse SourceMap: http://localhost:9001/libraries/backbone-min.map

Copy link
Contributor

@tomgreenfield tomgreenfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implicit +1 since was using it in issue/2647-2 branch.

Copy link

@lc-alexanderbenesch lc-alexanderbenesch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works 👌

@oliverfoster oliverfoster added the on hold do not merge label Mar 5, 2020
@oliverfoster
Copy link
Member Author

Awaiting a framework release and a few more reviews - just to be safe.

@brian-learningpool
Copy link
Member

Hi @oliverfoster, why exactly does CourseModel inherit MenuModel?

This was referenced Mar 6, 2020
@oliverfoster
Copy link
Member Author

oliverfoster commented Mar 6, 2020

@brian-learningpool

The CourseModel inherits from the MenuModel because CourseModel is both a menu and a contentobject (from the router's perspective), as well as the root container of the course content (no _parentId or siblings), the home of the _globals namespace (grunt extends the model, views and handlebars reference) and the target for plugin attributes (schemas define these), etc. It'll make simplifying the router a little easier later. The router should just manage contentObjects + bespoke plugin routes.

The inheritance was:
AdaptModel -> CourseModel

Now it's:
AdaptModel -> ContentObjectModel -> MenuModel -> CourseModel

To match:
AdaptModel -> ContentObjectModel -> MenuModel
AdaptModel -> ContentObjectModel -> PageModel

It means we don't have to lookup the _type=[menu/page/course], we can just check if it's a ContentObjectModel instance and the router is then allowed to render it straight into the wrapper. Otherwise if the thing the router has been asked to render is not a ContentObjectModel, we need to find the first ancestor ContentObjectModel, render that and scroll to the specified id.

I want to remove hard-coded references to plugin types and this is an easy win without changing anything too complicated, but at the same time it also allows us to simplify some really unnecessarily complicated code.

Plus it doesn't break anything 👍 and any subsequent behaviour we attach to the ContentObjectModel or the MenuModel won't have to be duplicated into the CourseModel. Like the introduction of contentObjectView:* events, for example, rather than having to do pageView:ready menuView:ready we can do contentObjectView:ready instead.

Stuff like this can be removed, moved or simplified without any impact on behaviour but with strides in clarity:
https://github.com/adaptlearning/adapt_framework/blob/master/src/core/js/models/adaptModel.js#L47-L52
https://github.com/adaptlearning/adapt_framework/blob/master/src/core/js/models/adaptModel.js#L648-L650
https://github.com/adaptlearning/adapt_framework/blob/master/src/core/js/router.js#L161-L162
https://github.com/adaptlearning/adapt_framework/blob/master/src/core/js/router.js#L177-L199

@oliverfoster oliverfoster removed the on hold do not merge label Mar 11, 2020
@oliverfoster
Copy link
Member Author

This pull request is ready for final +1s

@cahirodoherty-learningpool
Copy link
Contributor

Hi Oliver,
Thanks for checking in, but I think we are holding you up on this unfairly, so I'd say fire away with your implementation. Brian may get in touch to discuss your changes from a curiosity perspective, but we don't want to hold you up any further.
Apologies for that.

@oliverfoster oliverfoster merged commit 50c2892 into master Mar 19, 2020
@oliverfoster oliverfoster deleted the issue/2647 branch March 19, 2020 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add es6 support
10 participants