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.

Sign up for GitHub

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

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

Merged
merged 4 commits into from
Aug 26, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
339d0ba
handle wrapped HTMLBars AST plugins
eoneill Aug 18, 2021
05ba5c9
mirror cacheKey
eoneill Aug 18, 2021
0a5e4f2
mirror _parallelBabel = parallelBabel
eoneill Aug 25, 2021
3fdbfa1
enable THROW_UNLESS_PARALLELIZABLE=1 in stage1
eoneill Aug 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions packages/compat/src/prepare-htmlbars-ast-plugins.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { join } from 'path';

export default function prepHtmlbarsAstPluginsForUnwrap(registry: any): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be unit tested to prevent future regressions?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 [email protected] 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).

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@stefanpenner stefanpenner Aug 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

for (let wrapper of registry.load('htmlbars-ast-plugin')) {
const { plugin, parallelBabel, baseDir, cacheKey } = wrapper;
if (plugin) {
// if the parallelBabel options were set on the wrapper, but not on the plugin, add it
if (parallelBabel && !plugin.parallelBabel) {
plugin.parallelBabel = {
requireFile: join(__dirname, 'htmlbars-unwrapper.js'),
buildUsing: 'unwrapPlugin',
params: parallelBabel,
};
}

// NOTE: `_parallelBabel` (not `parallelBabel`) is expected by broccoli-babel-transpiler
if (plugin.parallelBabel && !plugin._parallelBabel) {
plugin._parallelBabel = plugin.parallelBabel;
}

// if the baseDir is set on the wrapper, but not on the plugin, add it
if (baseDir && !plugin.baseDir) {
plugin.baseDir = baseDir;
}

// if the cacheKey is set on the wrapper, but not on the plugin, add it
if (cacheKey && !plugin.cacheKey) {
plugin.cacheKey = cacheKey;
}
}
}
}
2 changes: 2 additions & 0 deletions packages/compat/src/v1-addon.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { ResolvedDep } from '@embroider/core/src/resolver';
import TemplateCompilerBroccoliPlugin from './template-compiler-broccoli-plugin';
import { fromPairs } from 'lodash';
import { getEmberExports } from '@embroider/core/src/load-ember-template-compiler';
import prepHtmlbarsAstPluginsForUnwrap from './prepare-htmlbars-ast-plugins';

const stockTreeNames = Object.freeze([
'addon',
Expand Down Expand Up @@ -132,6 +133,7 @@ export default class V1Addon {
if (options.plugins && options.plugins.ast) {
// our macros don't run here in stage1
options.plugins.ast = options.plugins.ast.filter((p: any) => !isEmbroiderMacrosPlugin(p));
prepHtmlbarsAstPluginsForUnwrap(this.addonInstance.registry);
if (options.plugins.ast.length > 0) {
const { cacheKey: compilerChecksum } = getEmberExports(options.templateCompilerPath);

Expand Down
19 changes: 2 additions & 17 deletions packages/compat/src/v1-app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import Concat from 'broccoli-concat';
import mapKeys from 'lodash/mapKeys';
import SynthesizeTemplateOnlyComponents from './synthesize-template-only-components';
import { isEmberAutoImportDynamic } from './detect-babel-plugins';
import prepHtmlbarsAstPluginsForUnwrap from './prepare-htmlbars-ast-plugins';

// This controls and types the interface between our new world and the classic
// v1 app instance.
Expand Down Expand Up @@ -560,23 +561,7 @@ export default class V1App {
// even if the app was using @embroider/macros, we drop it from the config
// here in favor of our globally-configured one.
options.plugins.ast = options.plugins.ast.filter((p: any) => !isEmbroiderMacrosPlugin(p));

// The parallelization protocol in ember-cli-htmlbars doesn't actually
// apply to the AST plugins, it applies to wrappers that
// ember-cli-htmlbars keeps around the plugins. Those wrappers aren't
// availble to us when we look at the template compiler configuration, so
// we need to find them directly out of the registry here. And we need to
// provide our own unwrapper shim to pull the real plugin out of the
// wrapper after deserializing.
for (let wrapper of this.app.registry.load('htmlbars-ast-plugin')) {
if (wrapper.parallelBabel && wrapper.plugin && !wrapper.plugin.parallelBabel) {
wrapper.plugin.parallelBabel = {
requireFile: join(__dirname, 'htmlbars-unwrapper.js'),
buildUsing: 'unwrapPlugin',
params: wrapper.parallelBabel,
};
}
}
prepHtmlbarsAstPluginsForUnwrap(this.app.registry);
}
return options.plugins;
}
Expand Down
2 changes: 2 additions & 0 deletions packages/compat/tests/stage1.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ describe('stage1 build', function () {
let expectFile: ExpectFile;

beforeAll(async function () {
process.env.THROW_UNLESS_PARALLELIZABLE = '1'; // see https://github.com/embroider-build/embroider/pull/924
// A simple ember app with no tests
let app = Project.emberNew();

Expand Down Expand Up @@ -107,6 +108,7 @@ describe('stage1 build', function () {
});

afterAll(async function () {
delete process.env.THROW_UNLESS_PARALLELIZABLE;
await build.cleanup();
});

Expand Down