-
Notifications
You must be signed in to change notification settings - Fork 10
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
Upgrade ember-in-element-polyfill #25
Conversation
05d157f
to
b35f7a0
Compare
This doesn't work IRL because of #17 I got our canary tests passing using tildeio/ember-maybe-in-element#resolve-in-element-deprecation, which is branched off of v0.2.0 (which I gather is the same as v0.4.0). |
@gitKrystan Phenomenal findings! Yes you were right. Feel free to merge master into your PR and we will merge this PR and cut 1.0.0! |
b35f7a0
to
b4cc5e9
Compare
@snewcomer Heads up, I spoke too soon on this fixing my canary build. I'm now seeing build errors on beta and canary. Going to look into it w/ @chancancode to see if it's from this package or something else. Lots of
|
b4cc5e9
to
d4b1ff9
Compare
@chancancode and I are looking into why canary tests are failing for this PR with The AST transform in the ember internals for The transform for Currently the flow is: It should be: We will work on a fix in glimmer, and this PR should pass on canary once that fix is merged in Ember. Alternatively, if the proposal in #10 (comment) is accepted, ember-maybe-in-element would not be affected by the glimmer issue at all. |
This fixes the issue described in DockYard/ember-maybe-in-element#25 (comment) tl;dr If an AST transform introduces the element, it doesn't go through the parser, so it's missing the required `guid` and `insertBefore` normalization. This commit moves the processing to a later stage to avoid this issue.
@snewcomer |
Fixes deprecation warnings on Ember canary: ``` The use of the private `{{-in-element}}` is deprecated, please refactor to the public `{{in-element}}`. ``` See ember-polyfills/ember-in-element-polyfill#81
d4b1ff9
to
58295d8
Compare
Sorry for the aside here, @gitKrystan, I’m struggling in CI with the capitalisation conflict you posted about:
Do you remember how you fixed that? I checked out the force push diff but I couldn’t even find any references to Moment in the lockfile so maybe it’s not the right diff 😯 |
@backspace the problem is moment had a bad release in 2.25, see jasonmit/ember-cli-moment-shim#183 (comment) Ensuring your lockfile is using 2.26 (2.24 works too I suppose). If the problem occurs when installing without a lockfile then it's probably that the CI cache was poisoned with the bad moment release. IME, you would have to very aggressively flush the caches because of the fallback semantics (if PR cache is deleted, it falls back to the branch cache, then the master cache, etc). That being said, I am also not sure why/what is actually installing moment here. |
Thanks for the context, @chancancode! Always good to get more understanding of mysterious failures. I ultimately grudgingly decided to flush the cache and it did indeed address the problem. |
Need anything else from me to get this ready for merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gitKrystan Thank you for this! Is heartily appreciated.
We tried to upgrade an app to v1.0 which includes this change and our builds are now failing with:
I assume it is related to this PR? 🤔 |
@Turbo87 it must be, but not sure why. I'll try to check it this afternoon. |
Fixes deprecation warnings on Ember canary:
See ember-polyfills/ember-in-element-polyfill#81