Avoid conflicts with ember-cli-htmlbars-inline-precompile #317
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rwjblue
added a commit
to rwjblue/ember-cli-htmlbars
that referenced
this pull request
Oct 1, 2019
…s-inline-precompile We **specifically** had tests for the scenario that ultimately was reported as failing. The description in ember-cli#317 explains the scenario better, but the failure scenario was only exposed IFF the same version of babel-plugin-htmlbars-inline-precompile was used (because both ember-cli-htmlbars-inline-precompile and ember-cli-htmlbars were comparing `require.resolve('babel-plugin-htmlbars-inline-precompile')` to determine if they should append the plugin again, so if they didn't use the same version ember-cli#312 wasn't possible). This updates the ember-try scenario to use ember-cli-htmlbars-inline-precompile@3.0.0, in order to replicate the error.
|
FYI - We had tests specifically for this but they didn't work 😭. I've updated those tests in #319 in order to replicate the original failure (and have confidence that this is fixing things). |
|
Turns out that I was wrong, |
Both ember-cli-htmlbars-inline-precompile and ember-cli-htmlbars used the same _exact_ check to determine if `babel-plugin-htmlbars-inline-precompile` was registered. When the build was parallelizable we would properly detect that the babel-plugin-htmlbars-inline-precompile was or was not the one added by each respective addon (because the addon's own `__dirname` is included in the check) **but** when the build was not parallelizable the babel plugins array contains a standard Babel plugins array. In that case, both addons were _only_ checking if the plugin _path_ matched (via `require.resolve`) which was completely insufficient. This issue resulted in `ember-cli-htmlbars-inline-precompile` _thinking_ that it was already registered _when_ the build was not parallelizable, which would subsequently make it not register its own instance of the plugin (to support import paths other than `ember-cli-htmlbars`). The fix here s twofold: 1. This forces any instances of ember-cli-htmlbars-inline-precompile addon to be completely inert if it is part of the same parent addon or app as ember-cli-htmlbars itself. In addition, we emit a deprecation notice when applicable. 2. Updating the `_isInlinePrecompileBabelPluginRegistered` method to properly check if the plugin ember-cli-htmlbars is registering is already registered (with our configuration).
rwjblue
force-pushed
the
prevent-conflicts-with-legacy-inline-compiler
branch
from
7c969ab
to
2c3121f
Compare
|
Updated this PR now to use a different strategy:
|
…s-inline-precompile We **specifically** had tests for the scenario that ultimately was reported as failing. The description in ember-cli#317 explains the scenario better, but the failure scenario was only exposed IFF the same version of babel-plugin-htmlbars-inline-precompile was used (because both ember-cli-htmlbars-inline-precompile and ember-cli-htmlbars were comparing `require.resolve('babel-plugin-htmlbars-inline-precompile')` to determine if they should append the plugin again, so if they didn't use the same version ember-cli#312 wasn't possible). This updates the ember-try scenario to use ember-cli-htmlbars-inline-precompile@3.0.0, in order to replicate the error. (cherry picked from commit 79e953e)
|
The last commit here cherry-pick's the commit from #319 and demonstrates that this PR fixes the issue. |
HeroicEric
added a commit
to HeroicEric/ember-cli
that referenced
this pull request
Oct 5, 2019
- Drops `ember-cli-htmlbars-inline-precompile` from both app and addon blueprints - Bumps `ember-cli-htmlbars` from [4.0.1](https://github.com/ember-cli/ember-cli-htmlbars/releases/tag/v4.0.1) to [4.0.5](https://github.com/ember-cli/ember-cli-htmlbars/releases/tag/v4.0.5) Why? Currently, generating a new ember app or addon and running it will result in the following deprecation: ``` DEPRECATION: ember-cli-htmlbars-inline-precompile is no longer needed with ember-cli-htmlbars versions 4.0.0 and higher, please remove it from `/Users/eric/code/foo/package.json` ``` See ember-cli/ember-cli-htmlbars#317 for more details. Steps to reproduce the current issue: ```bash npm install -g ember-cli ember new foo-app cd foo-app ember s ```
HeroicEric
added a commit
to HeroicEric/ember-cli
that referenced
this pull request
Oct 5, 2019
- Drops `ember-cli-htmlbars-inline-precompile` from both app and addon blueprints - Bumps `ember-cli-htmlbars` from [4.0.1](https://github.com/ember-cli/ember-cli-htmlbars/releases/tag/v4.0.1) to [4.0.5](https://github.com/ember-cli/ember-cli-htmlbars/releases/tag/v4.0.5) Why? Currently, generating a new ember app or addon and running it will result in the following deprecation: ``` DEPRECATION: ember-cli-htmlbars-inline-precompile is no longer needed with ember-cli-htmlbars versions 4.0.0 and higher, please remove it from `/Users/eric/code/foo/package.json` ``` See ember-cli/ember-cli-htmlbars#317 for more details. Steps to reproduce the current issue: ```bash npm install -g ember-cli ember new foo-app cd foo-app ember s ```
rwjblue
pushed a commit
to HeroicEric/ember-cli
that referenced
this pull request
Oct 7, 2019
- Drops `ember-cli-htmlbars-inline-precompile` from both app and addon blueprints - Bumps `ember-cli-htmlbars` from [4.0.1](https://github.com/ember-cli/ember-cli-htmlbars/releases/tag/v4.0.1) to [4.0.5](https://github.com/ember-cli/ember-cli-htmlbars/releases/tag/v4.0.5) Why? Currently, generating a new ember app or addon and running it will result in the following deprecation: ``` DEPRECATION: ember-cli-htmlbars-inline-precompile is no longer needed with ember-cli-htmlbars versions 4.0.0 and higher, please remove it from `/Users/eric/code/foo/package.json` ``` See ember-cli/ember-cli-htmlbars#317 for more details. Steps to reproduce the current issue: ```bash npm install -g ember-cli ember new foo-app cd foo-app ember s ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Both ember-cli-htmlbars-inline-precompile and ember-cli-htmlbars used the same exact check to determine if
babel-plugin-htmlbars-inline-precompilewas registered. When the build was parallelizable we would properly detect that the babel-plugin-htmlbars-inline-precompile was or was not the one added by each respective addon (because the addon's own__dirnameis included in the check) but when the build was not parallelizable the babel plugins array contains a standard Babel plugins array. In that case, both addons were only checking if the plugin path matched (viarequire.resolve) which was completely insufficient.This issue resulted in
ember-cli-htmlbars-inline-precompilethinking that it was already registered when the build was not parallelizable, which would subsequently make it not register its own instance of the plugin (to support import paths other thanember-cli-htmlbars).The fix here is:
ember-cli-htmlbars-inline-precompileshould be removed._isInlinePrecompileBabelPluginRegisteredmethod to properly check if the plugin ember-cli-htmlbars is registering is already registered (with our configuration).Fixes #312
Fixes #297
Closes #319
Fixes ember-cli/ember-cli#8868