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

add delayed transpilation #7501

Merged
merged 1 commit into from Feb 25, 2018

Conversation

Projects
None yet
6 participants
@kellyselden
Copy link
Member

kellyselden commented Dec 13, 2017

I would like some feedback. Things to note:

  • we have to detect files that are already AMD because certain addons do their own transpilation.
  • we have to detect addons in legacy 'modules/' dir and undo to transpile correctly.
  • removed treeForAddon/compileModules warning, because it is now the desired behavior.
  • removed compileTemplates no template processor warning, because it is now the desired behavior.

Sample console output:

DEPRECATION: Addon "@html-next/vertical-collection" (found at "C:\Users\kelly\Desktop\my-app\node_modules\@html-next\vertical-collection") is manually placing files in the legacy "modules" folder. Support for this will be removed in a future version.
DEPRECATION: Addon "ember-data" (found at "C:\Users\kelly\Desktop\my-app\node_modules\ember-data") is manually placing files in the legacy "modules" folder. Support for this will be removed in a future version.

Left to do:

@kellyselden kellyselden force-pushed the kellyselden:delay-transpilation-3 branch 2 times, most recently from 6db915c to 283f446 Dec 21, 2017

@rtablada

This comment has been minimized.

Copy link
Contributor

rtablada commented Dec 27, 2017

@kellyselden should the AMD Excluder be extracted into broccoli-funnel-amd-excluder? I'm not sure if that could potentially be helpful as a separate package rather than as part of Ember CLI?

Though I know that adds a maintenance cost too.

let source = fs.readFileSync(inputFilePath, 'utf8');

// ember-data and others compile their own source
if (source.indexOf('define(') === 0) {

This comment has been minimized.

@rtablada

rtablada Dec 27, 2017

Contributor

Should be fine, but I'm not 100 about potential edgecases. It would likely not be compatible JS anyway so this should be good enough to figure out if something is AMD.

On the other hand, what if the module is exported for AMD but doesn't define immediately.

This comment has been minimized.

@kellyselden

kellyselden Dec 27, 2017

Member

There are a lot of potential edge cases, but in reality we only have to worry about how babel formats the AMD code, which this should cover completely?

@kellyselden

This comment has been minimized.

Copy link
Member

kellyselden commented Dec 27, 2017

@rtablada I am in favor of extracting it.

@rtablada

This comment has been minimized.

Copy link
Contributor

rtablada commented Dec 27, 2017

I'd like to see what documentation (if any) is needed for addon authors to make sure their addons work with this.

Bonus points for docs on how to utilize these changes in addons.

@kellyselden

This comment has been minimized.

Copy link
Member

kellyselden commented Dec 27, 2017

@rtablada The only thing addons have to worry about is the first bullet point

addons no longer receive preprocessTree and postprocessTree hooks.

const Funnel = require('broccoli-funnel');
const walkSync = require('walk-sync');

class AmdExcluder extends Funnel {

This comment has been minimized.

@Turbo87

Turbo87 Dec 28, 2017

Member

can you use https://github.com/broccolijs/broccoli-test-helper to write a few tests for this class?

This comment has been minimized.

@Turbo87

Turbo87 Dec 28, 2017

Member

a little bit of documentation on this class might also be nice. e.g. answering the question that @rtablada raised about this being specialized to detect AMD modules compiled by Babel (5? 6? 7?)

This comment has been minimized.

@kellyselden

kellyselden Jan 27, 2018

Member

added docs

let files = walkSync(inputPath, {
directories: false,
globs: [`**/*.js`],
});

This comment has been minimized.

@Turbo87

Turbo87 Dec 28, 2017

Member

does build() have to be sync? otherwise it might make sense to read and process several files in parallel 🤔

This comment has been minimized.

@kellyselden

kellyselden Jan 27, 2018

Member

Doesn't have to be. Which package should I use for async walking?

This comment has been minimized.

@Turbo87

Turbo87 Jan 27, 2018

Member

I guess walkSync is okay, but the loop afterwards could possibly be concurrent

This comment has been minimized.

@kellyselden

kellyselden Jan 28, 2018

Member

Changed to async readFile. Was this what you were thinking?


// ember-data and others compile their own source
if (source.indexOf('define(') === 0) {
this.exclude.push(file);

This comment has been minimized.

@Turbo87

Turbo87 Dec 28, 2017

Member

will this fill up this.exclude with duplicate entries on rebuilds? we might have to reset this.exclude = []; at the beginning of build() 🤔

precompiledSource = preprocessJs(precompiledSource, '/', '/', {
annotation: `_compileAddonJs`,
registry: this.registry,
});

This comment has been minimized.

@Turbo87

Turbo87 Dec 28, 2017

Member

I'm wondering if in terms of performance it would be better to teach preprocessJs() about the AMD exclusion rule 🤔 but then again that is something that can probably be refactored later...

@kellyselden kellyselden force-pushed the kellyselden:delay-transpilation-3 branch 7 times, most recently from 4f09907 to 529d6af Jan 25, 2018

@kellyselden

This comment has been minimized.

Copy link
Member

kellyselden commented Jan 27, 2018

wooo tests finally green.

@kellyselden kellyselden force-pushed the kellyselden:delay-transpilation-3 branch 2 times, most recently from 614ab47 to 4670071 Jan 27, 2018

@kellyselden kellyselden force-pushed the kellyselden:delay-transpilation-3 branch 2 times, most recently from f7dc658 to d83a323 Jan 28, 2018

@kellyselden kellyselden requested review from rwjblue , Turbo87 and twokul Jan 28, 2018

@kellyselden kellyselden force-pushed the kellyselden:delay-transpilation-3 branch from d83a323 to 4132594 Jan 28, 2018

@twokul
Copy link
Contributor

twokul left a comment

Reviewed with @rwjblue (and @kellyselden / @twokul), this seems good with a couple of minor cleanup points...

`(NOT \`devDependencies\`) in \`${addon.name}\`'s \`package.json\`.`
);
});
// it('should throw a useful error if a template compiler is not present -- non-pods', function() {

This comment has been minimized.

@twokul

twokul Jan 29, 2018

Contributor

needs to be deleted

This comment has been minimized.

@kellyselden

kellyselden Feb 5, 2018

Member

deleted

`Please make sure your template precompiler (commonly \`ember-cli-htmlbars\`) is listed in \`dependencies\` ` +
`(NOT \`devDependencies\`) in \`${this.name}\`'s \`package.json\`.`);
}
// if (!registryHasPreprocessor(this.registry, 'template')) {

This comment has been minimized.

@twokul

twokul Jan 29, 2018

Contributor

needs to be deleted

This comment has been minimized.

@kellyselden

kellyselden Feb 5, 2018

Member

deleted

@@ -1079,7 +1116,9 @@ class EmberApp {
annotation: 'TreeMerger: `addon/` trees',
});

this._cachedAddonTree = new Funnel(combinedAddonTree, {
let compiledAddonTree = this._compileAddonTree(combinedAddonTree);

This comment has been minimized.

@twokul

twokul Jan 29, 2018

Contributor

Can we add some broccoli-debug trees for before and after? I'm mostly thinking this will be super useful for seeing where some transpilation issues might be coming from...

This comment has been minimized.

@kellyselden

kellyselden Feb 4, 2018

Member

I emulated the style from the assembler. @twokul you may want to verify.

@@ -617,6 +619,41 @@ class EmberApp {
}, []);
}

_compileAddonTemplates(tree) {
if (registryHasPreprocessor(this.registry, 'template')) {

This comment has been minimized.

@twokul

twokul Jan 29, 2018

Contributor

Can we actually get here?

This comment has been minimized.

@kellyselden

kellyselden Feb 4, 2018

Member

removed

This comment has been minimized.

@kellyselden

kellyselden Feb 6, 2018

Member

I remember why I added it. Without it we get errors in the styles tests https://travis-ci.org/ember-cli/ember-cli/jobs/337422087#L805-L813 missing template preprocessor. I will add more mocking to the styles tests.

This comment has been minimized.

@kellyselden
let inputFilePath = path.join(inputPath, file);
return fs.readFile(inputFilePath, 'utf8').then(source => {
if (source.indexOf('define(') === 0) {
this.exclude.push(file);

This comment has been minimized.

@twokul

twokul Jan 29, 2018

Contributor

Lets add a warning here...

This comment has been minimized.

@twokul

twokul Jan 29, 2018

Contributor

this.exclude needs to be reset here (and we probably need to clear whatever funnel caches have been build up)

This comment has been minimized.

@kellyselden

kellyselden Feb 8, 2018

Member

exclude is reset and it prints to console. the code has been moved to https://github.com/ember-cli/broccoli-amd-funnel. I also added tests.

}

_compileAddonJs(tree) {
let precompiledSource = new AmdExcluder(tree, {

This comment has been minimized.

@twokul

twokul Jan 29, 2018

Contributor

lets add a flag to opt-in or out of this tree as an option to new EmberApp, the default value should be true (e.g. use the amd excluder) but ultimately we should try to change it to false once a good number of popular addons have been updated...

This comment has been minimized.

@kellyselden
// treeForAddon without calling super
// they need the original params preserved because they call
// babel themselves and expect compilation the old way
this.options[emberCLIBabelConfigKey] = Object.assign({}, original, {

This comment has been minimized.

@twokul

twokul Jan 29, 2018

Contributor

This really should be moved back to the constructor if at all possible, setting it here means that others that depend on it earlier just don't have access to the values when they expect...

This comment has been minimized.

@kellyselden

kellyselden Feb 5, 2018

Member

I remember why I placed it here instead of the constructor. Having it in the constructor means I need to handle the legacy /modules using broccoli-module-normalizer. Making that change now.

This comment has been minimized.

@kellyselden

@kellyselden kellyselden force-pushed the kellyselden:delay-transpilation-3 branch 4 times, most recently from 5f3a5f6 to 3198d21 Feb 4, 2018

@kellyselden kellyselden force-pushed the kellyselden:delay-transpilation-3 branch from 94063b2 to a2bb5e3 Feb 18, 2018

@kellyselden kellyselden force-pushed the kellyselden:delay-transpilation-3 branch 4 times, most recently from 16af76f to 1113dc8 Feb 18, 2018

@kellyselden kellyselden changed the title WIP: add delayed transpilation add delayed transpilation Feb 21, 2018

@kellyselden kellyselden force-pushed the kellyselden:delay-transpilation-3 branch 6 times, most recently from 0bee743 to 2da17aa Feb 21, 2018

@kellyselden kellyselden force-pushed the kellyselden:delay-transpilation-3 branch 4 times, most recently from 2cd5918 to 7801f39 Feb 25, 2018

@kellyselden kellyselden force-pushed the kellyselden:delay-transpilation-3 branch from 7801f39 to 0aec861 Feb 25, 2018

@kellyselden

This comment has been minimized.

Copy link
Member

kellyselden commented Feb 25, 2018

All this new code is guarded around the feature flag DELAYED_TRANSPILATION. Because of this, and because @rwjblue and @twokul have reviewed this multiple times with no remaining problems, I think it is safe to merge.

@kellyselden kellyselden merged commit 3a78ec3 into ember-cli:master Feb 25, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 92.797%
Details

@kellyselden kellyselden deleted the kellyselden:delay-transpilation-3 branch Feb 25, 2018

@rwjblue rwjblue referenced this pull request Mar 1, 2018

Merged

Module Unification Addons #7490

5 of 7 tasks complete

@Turbo87 Turbo87 referenced this pull request Mar 23, 2018

Merged

Ember 3.1 Release Blog Post #3230

1 of 13 tasks complete
@stefanpenner

This comment has been minimized.

Copy link
Contributor

stefanpenner commented Apr 4, 2018

I believe this PR is breaking things, specifically we seem to trying to transpile handlebars templates that exist <addon>/src/<somefolder>/templates/model.hbs

@rwjblue

This comment has been minimized.

Copy link
Contributor

rwjblue commented Apr 4, 2018

@stefanpenner - This should have been fixed by #7735, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment