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

Resolve the template compiler from ember-source instead of project root #696

Closed
wants to merge 4 commits into from

Conversation

thoov
Copy link
Collaborator

@thoov thoov commented Feb 23, 2021

We cannot assume this will be resolvable from the project root as it might be symlinked. Instead we should get the addon instance and grab the template compiler location directly from it. This is technically a more stable location of the compiler instead of hard coding a location.

This becomes relevant in ember-cli's test suite as they symlink the entire node_modules directory into a per test tmp directory and looking for a hard coded location fails.

@rwjblue
Copy link
Collaborator

rwjblue commented Mar 1, 2021

@thoov - Ready for a rebase (should fix canary failures).

thoov added 4 commits March 1, 2021 09:05
We cannot assume this will be resolvable from the project root as it might be
symlinked. Instead we should the package location and grab the template
compiler from it.
@thoov thoov force-pushed the template-resolver-symlink branch from c6a1600 to f693f31 Compare March 1, 2021 17:06
@thoov
Copy link
Collaborator Author

thoov commented Mar 1, 2021

@rwjblue This is now green

@@ -317,10 +317,6 @@ class CompatAppAdapter implements AppAdapter<TreeNames> {
return this.configTree.readConfig().rootURL;
}

templateCompilerPath(): string {
return 'ember-source/vendor/ember/ember-template-compiler';
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like this is actually trying to resolve the path relative to the build itself. That is why it is ember-source/vendor/ember-template-compiler.js instead of where it actually is in the npm package ember-source/dist/ember-template-compiler.js.

This is where that is done:

https://github.com/emberjs/ember.js/blob/eff00a35624cd33f9f46f0991252182c367bb045/lib/index.js#L298-L300

Comment on lines +378 to +380
let emberSource = this.oldPackage.app.project.findAddonByName('ember-source');
let templateCompilerPath = emberSource.absolutePaths.templateCompiler;
return templateCompilerPath;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changes away from resolving the template compiler relative to the vendor asset output to resolving it in normal node modules resolution system.

This means that it would be no longer possible for folks to provide a custom / overridden ember-template-compiler asset (before all they needed to do was put it in the right location in their vendor folder).

I think this might be perfectly fine, but I'm not 100% sure...

@rwjblue
Copy link
Collaborator

rwjblue commented Mar 1, 2021

I debugged this with @thoov this afternoon, and we figured out what is going on. In ember-cli's own test suite (in order to avoid multiple invocations of npm install) the setup of some of the tests symlinks the entire node_modules folder. Embroider already had code (in packages/compat/src/moved-package-cache.ts) that handles symlinked packages, but it only handles if the individual packages themselves were symlinked (not the node_modules folder, or a full node_modules/@foo scope folder).

We have an alternative fix that works better in those cases (allowing the existing symlinked module dereferencing to be smarter).

@rwjblue
Copy link
Collaborator

rwjblue commented Mar 1, 2021

Closing in favor of #702

@rwjblue rwjblue closed this Mar 1, 2021
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.

2 participants