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

Ensure AST plugins have the same ordering as < ember-cli-htmlbars@5.5.0. #665

Merged
merged 1 commit into from
Feb 27, 2021

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Feb 27, 2021

We have to reverse these for reasons that are a bit bonkers. The initial version of this system used registeredPlugin from ember-template-compiler.js to set up these plugins (because Ember ~ 1.13 only had registerPlugin, and there was no way to pass plugins directly to the call to compile/precompile). Calling registerPlugin unfortunately inverted the order of plugins (it essentially did PLUGINS = [plugin, ...PLUGINS]).

Sooooooo...... we are forced to maintain that absolutely bonkers ordering.

This should address #664, but I'd like to leave it open so @simonihmig can confirm.

We have to reverse these for reasons that are a bit bonkers. The initial
version of this system used `registeredPlugin` from
`ember-template-compiler.js` to set up these plugins (because Ember ~
1.13 only had `registerPlugin`, and there was no way to pass plugins
directly to the call to `compile`/`precompile`). Calling
`registerPlugin` unfortunately **inverted** the order of plugins (it
essentially did `PLUGINS = [plugin, ...PLUGINS]`).

Sooooooo...... we are forced to maintain that **absolutely bonkers**
ordering.
@rwjblue rwjblue added the bug label Feb 27, 2021
@rwjblue
Copy link
Member Author

rwjblue commented Feb 27, 2021

@rwjblue rwjblue merged commit 6c0eaf6 into master Feb 27, 2021
@delete-merged-branch delete-merged-branch bot deleted the properly-inverse-plugins-wtfbbq branch February 27, 2021 04:19
@lukemelia
Copy link

Is there a list somewhere we can add this to so we make it rational in a breaking change for Ember 4.0? Or are stuck with this forever?

@rwjblue
Copy link
Member Author

rwjblue commented Mar 12, 2021

@lukemelia - Hmm. I would really like to find a way to do it, but I don't know of a reasonable path forward. We could easily guard this reversing code (and the code in Embroider that I linked) to avoid reversing when running against Ember >= 4, but then any addon that wants to support both Ember < 4 and 4 will have no way to actually guarantee the ordering.

@rwjblue
Copy link
Member Author

rwjblue commented Mar 12, 2021

Ironically, with Ember 4 the reason for reversing will actually be gone (since the registerPlugin based API is deprecated). 😭

@rwjblue
Copy link
Member Author

rwjblue commented Mar 12, 2021

@lukemelia - Mind making an issue to track the discussion / brain storming? Seems fine to do here, emberjs/rfcs, emberjs/ember.js, or Embroider repos...

@lukemelia
Copy link

@rwjblue created emberjs/rfcs#725

@rwjblue
Copy link
Member Author

rwjblue commented Mar 16, 2021

Thank you!

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.

None yet

2 participants