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

Replace Esperanto With Babel #4764

Closed
wants to merge 5 commits into from

Conversation

chadhietala
Copy link
Member

  • Remove broccoli-es6modules
  • Skip funnel and transpilation if babel is installed in user land
  • Hookup to an app to verify.

This makes babel required. The next step after this lands is to figure out what Class is referring to come up with a better concat strategy as it appears to be an issue.

Final

Before

After

@stefanpenner
Copy link
Contributor

:dances:

@rwjblue
Copy link
Member

rwjblue commented Aug 26, 2015

Just to make sure I understand properly, amd-name-resolver is used to ensure no relative paths end up in the transpiled output?

@chadhietala
Copy link
Member Author

@rwjblue yes

@rwjblue
Copy link
Member

rwjblue commented Aug 26, 2015

I believe that ember-cli-babel disables es6.modules via a blacklist here unless specifically present in the config.

modules: 'amdStrict',
moduleIds: true,
exportModuleMetadata: true,
sourceMaps: !isProduction,
Copy link
Member

Choose a reason for hiding this comment

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

This should be based on options.sourcmaps.enabled (which is defaulted to !isProduction below but can be enabled and disabled independently of a production build).

@rwjblue
Copy link
Member

rwjblue commented Aug 26, 2015

I am very excited about this! Thanks for spearheading @chadhietala!

@chadhietala
Copy link
Member Author

From what i gathered you have to set compileModules to true to get around this in ember-cli-babel. See here. I still have mega doubts that the tests are going to pass :P

@chadhietala chadhietala force-pushed the prefer-babel branch 2 times, most recently from fb9e93d to 2f069c8 Compare August 27, 2015 01:09
@chadhietala chadhietala changed the title [WIP] Use Babel if installed [WIP] Use Babel Aug 27, 2015
@chadhietala
Copy link
Member Author

No idea how any tests are passing on node 0.10.0 due to the fact broccoli-sri-hash uses a native promise.

@chadhietala chadhietala changed the title [WIP] Use Babel Use Babel Aug 27, 2015
@chadhietala
Copy link
Member Author

Update:

Need to do further investigation as the trees created in _addAppTests need to be babelified. Currently cannot get an app booted.

@joostdevries
Copy link
Member

This completely removes esperanto right? So module transpilation is done using Babel? Maybe update the PR title to reflect that?

Also: very awesome.

@rwjblue
Copy link
Member

rwjblue commented Aug 27, 2015

@joostdevries - It only removes the usage of Esperanto if the project has ember-cli-babel installed. Due to various reasons (@stefanpenner can elaborate) we do not require apps to use ember-cli-babel.

@stefanpenner / @chadhietala - I believe that master is ultimately going to become ember-cli 2.0.0, I think we should change to requiring ember-cli-babel in 2.0.0. This would be a breaking change (hence my desire to include it in 2.0.0, but largely not affect the majority of our users since ember-cli-babel is included in all blueprinted apps. Thoughts?

@stefanpenner
Copy link
Contributor

@rwjblue yes agreed

@chadhietala
Copy link
Member Author

This PR actually drops esperanto all together and uses babel for the transpilation if they do not have ember-cli-babel installed. So if you do or do not have ember-cli-babel it should be transparent. We can go 1 step further and whitelist es6.modules in the event the app does not have ember-cli-babel. Hoping to get this fixed and merged today, sourcemaps seemed to be only half working last night but I was pleasantly surprised when I saw the actual source in dev tools.

@chadhietala chadhietala changed the title Use Babel Replace Esperanto With Babel Aug 27, 2015
@rwjblue
Copy link
Member

rwjblue commented Aug 27, 2015

This PR actually drops esperanto all together and uses babel for the transpilation if they do not have ember-cli-babel installed

Nice! Must have been added after I reviewed the diff initially...

If we intend to support apps without ember-cli-babel, then we need some slow acceptance tests to confirm that works. I personally think that we should just require that ember-cli@2.0.0 apps have ember-cli-babel. If that is what we intend to do, we should add a deprecation on top of 1.13.8 and release as 1.13.9 to make it clear that this will be removed in 2.0.0.

@chadhietala
Copy link
Member Author

I'm up for making it required and ember-cli will just have some sane defaults for babel. I'm currently dealing with the fact that some addons were relying on the fact that in the post process hook they had un-processed files. In our app this is ember-browserify. Will probably need to PR and fix this as modules will be completely transpiled once they enter this hook.

@chadhietala
Copy link
Member Author

So I verified that we will save ~5sec on a rebuild in a pathological case.

Before

After

I would have expected more but could be our app ¯_(ツ)_/¯ However these Babel numbers look really strange.

@rwjblue and @stefanpenner not sure what is going on with CI. Feel free to pull this locally and test.

@chadhietala
Copy link
Member Author

After the latest commit I identified where some double work was occurring. Here is the new rebuild time so the savings looks to be close to ~8sec which is the approximate time ES6 and App JS Files were taking.

@stefanpenner
Copy link
Contributor

20% - 25% win in our big app – and its test suite passes.

@stefanpenner
Copy link
Contributor

rebased and stuff here: #4792

@jcope2013
Copy link
Contributor

so is the assumption of this PR that ember-cli master is now required to use ember-cli-babel? It seems taking out ember-cli-babel will make an app no longer work properly due to

Uncaught ReferenceError: exports is not defined

screen shot 2015-11-01 at 12 41 50 pm

ember new foo
cd foo
npm link ember-cli
remove ember-cli-babel from package.json
rm -rf node_modules/ember-cli-babel/
ember server

@stefanpenner
Copy link
Contributor

so is the assumption of this PR that ember-cli master is now required to use ember-cli-babel?

It bundles its own.

but that output looks like the wrong output, @chadhietala can you take a look?

@chadhietala
Copy link
Member Author

@stefanpenner I can take a look, but I had been using the master branch locally for a while with no issues.

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.

None yet

5 participants