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

Enforce correct plugin order in addon-dev #2136

Merged
merged 3 commits into from
Oct 8, 2024
Merged

Conversation

simonihmig
Copy link
Collaborator

@simonihmig simonihmig commented Oct 2, 2024

The recent release of addon-dev@v6 was bringing in an undocumented breaking change (which by itself would be ok, since this was a new major) as #2121 required a specific plugin order, where babel has to come last, or at least after hbs and gjs.

The first commit here aligned the plugin order in tests to what we have in the addon-blueprint, where babel comes before hsb and gjs. I think we should keep these in sync, as that would have uncovered the breaking change!

After that change, you can see here how the babel plugin fails to parse the (untransformed) gjs file. Same happens in the PR to update deps in the addon-blueprint: embroider-build/addon-blueprint#299

We could tell users to change the plugin order, but that causes a lot of churn, as basically every v2 addon out there has the order as in the blueprint. And I think ideally from DX perspective would be good if the order didn't matter to users.

This PR enforces the hbs and gjs plugins to run before others (including babel).

Btw, for the hbs plugin, I believe this shouldn't really be needed in 99%+ of cases, only when you have hbs transforms registered via babel-plugin-ember-template-compilation, which we had in our tests.


babel({ babelHelpers: 'bundled' }),

addon.hbs(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think babel must be the last. whats the issue with the order?

Copy link
Contributor

@patricklx patricklx Oct 2, 2024

Choose a reason for hiding this comment

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

ah, I see, thats how the blueprint is.
But the recent PR changed hbs & gjs plugins to use transform hook. So the order of the blueprint must be changed.
the tests in embroider were already at correct order before the change... I wonder why it wasnt the same order in the blueprint

Copy link
Contributor

@patricklx patricklx Oct 2, 2024

Choose a reason for hiding this comment

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

looks like the PR i did was more of a breaking change then?
ah, but it was included into a breaking release anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, exactly. It should have at least a notice on the breaking change in the changelog. But I think it could be fixed maybe, trying this out in this draft PR! Will ping you once I'm ready.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, the blueprint should really have the babel plugin as last.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@patricklx ready to go now, see the PR description!

well, the blueprint should really have the babel plugin as last.

We can still decide to make that change in the blueprint if we feel this is the safer path forward. But the change in this PR makes this optional, which I think is better than forcing this on users by running into build errors...

@simonihmig simonihmig marked this pull request as ready for review October 2, 2024 13:47
@simonihmig simonihmig requested a review from a team October 2, 2024 13:49
@simonihmig simonihmig changed the title Fix addon-dev gjs plugin order Enforce correct plugin order in addon-dev Oct 2, 2024
@simonihmig simonihmig added the bug Something isn't working label Oct 2, 2024
@simonihmig
Copy link
Collaborator Author

Weird, beta tests (Ember 6) are failing here with Components with separately resolved templates were removed at Ember 6.0. Migrate to either co-located js/ts + hbs files or to gjs/gts., while they are passing for canary, and last scheduled job is also passing. But the changes here cannot have caused this (these are app tests), right? 🤔

@ef4
Copy link
Contributor

ef4 commented Oct 2, 2024

Canary is not really the current canary. #2073

@simonihmig
Copy link
Collaborator Author

while they are passing for canary, and last scheduled job is also passing

nvm, the passing scheduled job was for main. There was a previous failing build on stable before, so this is an issue we have on stable, and not related to this PR!

@simonihmig simonihmig merged commit 1ac0d49 into stable Oct 8, 2024
221 checks passed
@simonihmig simonihmig deleted the fix-addon-dev-gjs-order branch October 8, 2024 17:04
@github-actions github-actions bot mentioned this pull request Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants