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

Remove ember-cli-htmlbars dependency in @embroider/compat. #706

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

rwjblue
Copy link
Collaborator

@rwjblue rwjblue commented Mar 4, 2021

When introduced in 48fc86b this dependency was used (by packages/compat/src/apply-ast-transforms.ts) to import and re-use the broccoli plugin, but that code was removed in 07d3c46.

The only import we had for it was specifically for types, which we define internally (in types/ember-cli-htmlbars/index.d.ts).

These dependencies do not seem to be needed anymore.

@rwjblue rwjblue requested a review from ef4 March 4, 2021 00:10
@rwjblue rwjblue changed the title Remove ember-cli-htmlbars dependency in @embroider/compat. Remove ember-cli-htmlbars dependency in @embroider/compat. Mar 4, 2021
@rwjblue rwjblue force-pushed the remove-ember-cli-htmlbars-dep branch from d67ddb8 to 6ea80b7 Compare March 4, 2021 00:17
@rwjblue rwjblue marked this pull request as draft March 4, 2021 19:49
@rwjblue
Copy link
Collaborator Author

rwjblue commented Mar 8, 2021

Just to note the status here for anyone else stumbling by, this PR is not quite correct. There are tests in packages/compat's test suite that do require ember-cli-htmlbars-3, and those tests should have failed with this change (but didn't). We need to figure out why they still passed, then we can fix this PR by moving these down to devDependencies (which is the correct thing here, due to the tests).

We still need `ember-cli-htmlbars@3` for tests (specifically to ensure
that inline precompilation works still in that mode), but we definitely
do not need to ship this dependency to our consumers.

The only import we had for `ember-cli-htmlbars` itself was specifically
for types, which we define internally (in
`types/ember-cli-htmlbars/index.d.ts`).
@rwjblue rwjblue force-pushed the remove-ember-cli-htmlbars-dep branch from 6ea80b7 to f8a9e5f Compare March 8, 2021 17:42
Copy link
Collaborator Author

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

OK, so I've gotten to the bottom of why this was passing. Apparently, removing the aliasing in packages/compat/package.json then running yarn was "not enough" to remove the alias.

I've removed the name alias from yarn.lock (it seems newer versions of yarn@1 do not preserve name if it isn't needed), confirmed tests failed properly, then brought it back.

yarn.lock Outdated
@@ -6832,7 +6821,18 @@ ember-cli-htmlbars-inline-precompile@^2.1.0:
heimdalljs-logger "^0.1.9"
silent-error "^1.1.0"

ember-cli-htmlbars@^4.0.0, ember-cli-htmlbars@^4.0.8, ember-cli-htmlbars@^4.0.9, ember-cli-htmlbars@^4.2.0, ember-cli-htmlbars@^4.2.2, ember-cli-htmlbars@^4.2.3, ember-cli-htmlbars@^4.3.1:
ember-cli-htmlbars@^3.0.0, ember-cli-htmlbars@^3.0.1, ember-cli-htmlbars@^3.1.0:
name ember-cli-htmlbars-3
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This name alias is what is causing the tests to continue to pass. Someone in the build has ember-cli-htmlbars@3 still, and when yarn was ran after removing the alias it did not remove the name field here.

Removing this name field makes the tests properly fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant