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

Ensure ember-data/-private module is emitted properly for ember-cli < 2.12 #5022

Merged
merged 1 commit into from
Jun 19, 2017

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jun 19, 2017

ember-cli@2.12 and higher have the addon itself responsible for transpiling modules so that the output of the treeForAddon hook is expected to be AMD.

Prior versions of ember-cli (i.e. < 2.12) do second pass (that is not possible to disable) to transpile the output of treeForAddon again (to transpile modules).

This commit changes two things:

  • It leverages ember-cli-babel@6's built in ability to detect the various ember-cli version scenarios and either transpile ember-data/-private down to AMD or not correctly
  • It emits the output file from broccoli-rollup into modules/ember-data/-private.js (previously was modules/-private.js). In ember-cli < 2.12, we have to ensure that the emitted file is in the correct location for that second pass module transpilation.

… < 2.12.

ember-cli@2.12 and higher have the addon itself responsible for transpiling
modules so that the output of the `treeForAddon` hook is expected to be
AMD.

Prior versions of ember-cli (i.e. < 2.12) do second pass (that is not possible
to disable) to transpile the output of `treeForAddon` **again** (to transpile
modules).

This commit changes things around leveraging ember-cli-babel@6's built in
ability to detect the various ember-cli version scenarios and either
transpile `ember-data/-private` down to AMD or not correctly.
@rwjblue
Copy link
Member Author

rwjblue commented Jun 19, 2017

This will need to be ported forward to beta and master (it targets release branch now). I made the changes directly against 2.14.1, and master has diverged somewhat significantly (in a good way) so my patch didn't apply.


privateTree = new Rollup(privateTree, {
rollup: {
entry: '-private/index.js',
targets: [
{ dest: '-private.js', format: 'amd', moduleId: 'ember-data/-private' }
{ dest: 'ember-data/-private.js', format: babel.shouldCompileModules() ? 'amd' : 'es', moduleId: 'ember-data/-private' }
Copy link
Contributor

Choose a reason for hiding this comment

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

should dest be subject to the same conditional as format?

Copy link
Member Author

Choose a reason for hiding this comment

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

In 2.12 and higher we simply concat regardless of source path, so this is fine. In < 2.12, we need to be in the right place so that the transpiled AMD module has the right moduleId.

If you notice down below, the public modules are emitted into modules/ember-data already (in all cases). This should be fine for both sides (and the tests are passing).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks :)

Copy link
Member

@stefanpenner stefanpenner left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry I missed this scenario in my original implementation. Good catch @rwjblue!!

@bmac bmac merged commit c79ba09 into emberjs:release Jun 19, 2017
@rwjblue rwjblue deleted the fix-ember-cli-lt-2-12 branch June 20, 2017 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants