Fix caching of template AST plugins (follow caching protocol of ember-cli-htmlbars) #924

eoneill · 2021-08-18T23:04:16Z

This addresses two issues.

The first is that most registered htmlbars-ast-plugin will currently opt-out of caching becaue there is no caching strategy (baseDir/cacheKey) on the raw plugin. In classic build, this is derived from the plugin wrapper. In embroider, that context is currently lost. The fix here is to mirror baseDir/cacheKey onto the plugin itself.

The second is that we only "unwrap" plugins at the App level, not at the Addon level. This resulted in our builds failing when THROW_UNLESS_PARALLELIZABLE=1. The fix here is to also unwrap the plugins in the V1Addon.

The two changes allow our app to build with THROW_UNLESS_PARALLELIZABLE=1 and we no longer get a bunch of...

broccoli-babel-transpiler is opting out of caching due to a plugin that does not provide a caching strategy ...

NOTE: I changed plugin.parallelBabel to plugin._parallelBabel because that's what broccoli-babel-transpiler is expecting, but I'm not entirely sure this is correct (given the previous commit). If we need both for compat, we could mirror this...

plugin._parallelBabel = plugin.parallelBabel;

References:

stefanpenner · 2021-08-18T23:41:33Z

What can we do to prevent regressing this again? Is there a test scenario to add, or unit test to write? Obviously we want to the regression prevention to be as light wait as possible

stefanpenner · 2021-08-18T23:43:29Z

@eoneill the current test failures are unrelated to you work, I will be investigating those myself now as part of #925

ef4 · 2021-08-19T18:48:41Z

Yeah, I don't think we should be renaming the parallelBabel property. It's not being read by ember-cli-babel directly, it's being read by ember-cli-htmlbars which does expect parallelBabel and not _parallelBabel.

Otherwise this looks good to me.

stefanpenner · 2021-08-19T21:13:58Z

packages/compat/src/prepare-htmlbars-ast-plugins.ts

@@ -0,0 +1,28 @@
+import { join } from 'path';
+
+export default function prepHtmlbarsAstPluginsForUnwrap(registry: any): void {


should this be unit tested to prevent future regressions?

I would suggest setting THROW_UNLESS_PARALLELIZABLE during something like stage1.test.ts.

We already run with this flag, but only in the macros package's test suite.

Setting THROW_UNLESS_PARALLELIZABLE=1 in stage1.test.ts definitely causes the tests to fail. I was able to confirm that the patch here works. However, even with this patch, the tests continue to fail as the inject-babel-helpers plugin (from ember-source) is not parallelizable.

This was fixed in emberjs/ember.js#19047, but not backported to ember-source@3.17 which is currently used in a few test-packages (e.g. here and here).

Bumping ember-source resolves the issue, however, I don't think I can currently bump these instances of ember-source above 3.17 per #927 / #934 (@stefanpenner).

Bumping ember-source resolves the issue, however, I don't think I can currently bump these instances of ember-source above 3.17 per #927 / #934 (@stefanpenner)

@eoneill #927 / #934 are not ember-source, they are for ember-cli, but the fix appears to be in ember-source -> emberjs/ember.js@104265a

Looking at the ember-source commit, it suggests that if we get to ember-source@~3.22 the issue you describe is addressed.

rg 'ember-source":' test-packages/support/package.json 21: "ember-source": "3.17.0", packages/router/package.json 63: "ember-source": "~3.16.0", test-packages/macro-sample-addon/package.json 54: "ember-source": "~3.10.0", test-packages/fastboot-addon/package.json 47: "ember-source": "~3.17.0", test-packages/sample-transforms/package.json 46: "ember-source": "~3.10.0", packages/util/package.json 64: "ember-source": "~3.21.1", tests/app-template/package.json 54: "ember-source": "~3.26.1", tests/addon-template/package.json 53: "ember-source": "~3.26.1",

I can quickly try to upgrade those, and see if we can land this with the flag enabled

Let's see if we can just quickly sneak in a conservative ember-source bump to help with (pr open #935)

I've updated ember-source to the minimum version described ^^. You should be unblocked from that side of things

Rebased and added THROW_UNLESS_PARALLELIZABLE=1 in latest commit. This should be resolved now.

stefanpenner · 2021-08-19T21:28:33Z

Several of your tests that where failing due to the ember global being missing should now be green: #925

eoneill · 2021-08-20T22:29:48Z

Yeah, I don't think we should be renaming the parallelBabel property. It's not being read by ember-cli-babel directly, it's being read by ember-cli-htmlbars which does expect parallelBabel and not _parallelBabel.

Otherwise this looks good to me.

If I don't set _parallelBabel, it fails with...

[Babel: foo-addon] was configured to `throwUnlessParallelizable` and was unable to parallelize a plugin. 
plugins:
1: name: unknown, location: unknown

Please see: https://github.com/babel/broccoli-babel-transpiler#parallel-transpilation for more details
    at new Babel (/workspace/node_modules/broccoli-babel-transpiler/index.js:90:11)
    at Class.transpileTree (/workspace/node_modules/ember-cli-babel/index.js:169:16)
    at Object.toTree (/workspace/node_modules/ember-cli-babel/index.js:179:30)
    at /workspace/node_modules/ember-cli-preprocess-registry/preprocessors.js:188:26
    at Array.forEach (<anonymous>)
    at processPlugins (/workspace/node_modules/ember-cli-preprocess-registry/preprocessors.js:186:11)
    at Function.module.exports.preprocessJs (/workspace/node_modules/ember-cli-preprocess-registry/preprocessors.js:179:10)
    at Class.preprocessJs (/workspace/node_modules/ember-cli/lib/models/addon.js:1297:25)
    at V1Addon.transpile (/workspace/node_modules/@embroider/compat/src/v1-addon.js:407:35)
    at V1Addon.treeForAddon (/workspace/node_modules/@embroider/compat/src/v1-addon.js:605:25)
    at V1Addon.buildTreeForAddon (/workspace/node_modules/@embroider/compat/src/v1-addon.js:630:25)
    at V1Addon.build (/workspace/node_modules/@embroider/compat/src/v1-addon.js:891:14)
    at V1Addon.<anonymous> (/workspace/node_modules/typescript-memoize/dist/memoize-decorator.js:100:52)
    at V1Addon.get v2Trees (/workspace/node_modules/@embroider/compat/src/v1-addon.js:491:30)
    at V1Addon.<anonymous> (/workspace/node_modules/typescript-memoize/dist/memoize-decorator.js:100:52)
    at /workspace/node_modules/@embroider/compat/src/v1-addon.js:478:64
    at V1Addon.throughTreeCache (/workspace/node_modules/@embroider/compat/src/v1-addon.js:521:20)
    at V1Addon.get v2Tree [as v2Tree] (/workspace/node_modules/@embroider/compat/src/v1-addon.js:463:21)
    at /workspace/node_modules/@embroider/compat/src/build-compat-addon.js:42:48
    at Array.map (<anonymous>)
    at buildCompatAddon (/workspace/node_modules/@embroider/compat/src/build-compat-addon.js:42:33)
    at Object.cachedBuildCompatAddon [as default] (/workspace/node_modules/@embroider/compat/src/build-compat-addon.js:13:16)
    at /workspace/node_modules/@embroider/compat/src/compat-addons.js:46:105
    at Array.map (<anonymous>)
    at CompatAddons.get tree [as tree] (/workspace/node_modules/@embroider/compat/src/compat-addons.js:46:63)
    at CompatApp.augment (/workspace/node_modules/@embroider/core/src/build-stage.js:57:64)
    at CompatApp.get tree (/workspace/node_modules/@embroider/core/src/build-stage.js:26:50)
    at CompatApp.<anonymous> (/workspace/node_modules/typescript-memoize/dist/memoize-decorator.js:100:52)

This is because broccoli-babel-transpiler walks the entire object and recurses. When it hits the plugins.ast (which is an Array<Function>, it fails because the plugin Functions do not have a _parallelBabel option on them.
See broccoli-babel-transpiler/lib/parallel-api.js

I think the reasonable fix forwards is...

// set `parallelBabel` derived from wrapper
if (parallelBabel && !plugin.parallelBabel) {
  plugin.parallelBabel = { ... };
}
// mirror onto `_parallelBabel`
if (plugin.parallelBabel && !plugin._parallelBabel) {
  plugin._parallelBabel = plugin.parallelBabel;
}

ef4 · 2021-08-21T19:16:34Z

Ok, that makes sense.

This aims to help enable #924 (comment) to add THROW_UNLESS_PARALLELIZABLE

stefanpenner

Now that ember-source has been bumped, let's include THROW_UNLESS_PARALLELIZABLE in this PR to prevent regression as @ef4 recommended

stefanpenner · 2021-08-26T15:44:53Z

@eoneill Nice thanks!

eoneill changed the title ~~Compat htmlbars ast plugin wrapper~~ handle wrapped HTMLbars plugins Aug 19, 2021

stefanpenner requested changes Aug 19, 2021

View reviewed changes

stefanpenner approved these changes Aug 25, 2021

View reviewed changes

stefanpenner added a commit that referenced this pull request Aug 25, 2021

bump ember-source to at-least ~3.22.0

cef9b40

This aims to help enable #924 (comment) to add THROW_UNLESS_PARALLELIZABLE

stefanpenner mentioned this pull request Aug 25, 2021

Bump ember-source in test scenarios to at-least ~3.22.0 #935

Merged

stefanpenner added a commit that referenced this pull request Aug 25, 2021

bump ember-source to at-least ~3.22.0 (#935)

219f154

This aims to help enable #924 (comment) to add THROW_UNLESS_PARALLELIZABLE

stefanpenner requested changes Aug 25, 2021

View reviewed changes

eoneill added 4 commits August 25, 2021 13:18

handle wrapped HTMLBars AST plugins

339d0ba

mirror cacheKey

05ba5c9

mirror _parallelBabel = parallelBabel

0a5e4f2

enable THROW_UNLESS_PARALLELIZABLE=1 in stage1

3fdbfa1

eoneill force-pushed the compat-htmlbars-ast-plugin-wrapper branch from d4eedc8 to 3fdbfa1 Compare August 25, 2021 20:40

stefanpenner approved these changes Aug 26, 2021

View reviewed changes

stefanpenner merged commit 25f368a into embroider-build:master Aug 26, 2021

rwjblue added the bug Something isn't working label Aug 26, 2021

rwjblue changed the title ~~handle wrapped HTMLbars plugins~~ Fix caching of template AST plugins (follow caching protocol of ember-cli-htmlbars) Aug 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix caching of template AST plugins (follow caching protocol of ember-cli-htmlbars) #924

Fix caching of template AST plugins (follow caching protocol of ember-cli-htmlbars) #924

eoneill commented Aug 18, 2021

stefanpenner commented Aug 18, 2021

stefanpenner commented Aug 18, 2021

ef4 commented Aug 19, 2021

stefanpenner Aug 19, 2021

ef4 Aug 19, 2021

eoneill Aug 25, 2021

stefanpenner Aug 25, 2021

stefanpenner Aug 25, 2021 •

edited

Loading

stefanpenner Aug 25, 2021

eoneill Aug 26, 2021

stefanpenner commented Aug 19, 2021

eoneill commented Aug 20, 2021

ef4 commented Aug 21, 2021

stefanpenner left a comment

stefanpenner commented Aug 26, 2021

		@@ -0,0 +1,28 @@
		import { join } from 'path';

		export default function prepHtmlbarsAstPluginsForUnwrap(registry: any): void {

Fix caching of template AST plugins (follow caching protocol of ember-cli-htmlbars) #924

Fix caching of template AST plugins (follow caching protocol of ember-cli-htmlbars) #924

Conversation

eoneill commented Aug 18, 2021

stefanpenner commented Aug 18, 2021

stefanpenner commented Aug 18, 2021

ef4 commented Aug 19, 2021

stefanpenner Aug 19, 2021

Choose a reason for hiding this comment

ef4 Aug 19, 2021

Choose a reason for hiding this comment

eoneill Aug 25, 2021

Choose a reason for hiding this comment

stefanpenner Aug 25, 2021

Choose a reason for hiding this comment

stefanpenner Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

stefanpenner Aug 25, 2021

Choose a reason for hiding this comment

eoneill Aug 26, 2021

Choose a reason for hiding this comment

stefanpenner commented Aug 19, 2021

eoneill commented Aug 20, 2021

ef4 commented Aug 21, 2021

stefanpenner left a comment

Choose a reason for hiding this comment

stefanpenner commented Aug 26, 2021

stefanpenner Aug 25, 2021 •

edited

Loading