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

Upgrade ember-in-element-polyfill #25

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

gitKrystan
Copy link
Contributor

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

@snewcomer snewcomer mentioned this pull request Apr 29, 2020
@gitKrystan
Copy link
Contributor Author

I pulled #26 into this PR to get CI running, but we can always drop that commit once #26 is merged.

@gitKrystan gitKrystan force-pushed the upgrade-polyfill branch 2 times, most recently from 05d157f to b35f7a0 Compare April 30, 2020 22:35
@gitKrystan
Copy link
Contributor Author

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).

@snewcomer snewcomer mentioned this pull request May 1, 2020
@snewcomer
Copy link
Contributor

@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!

@gitKrystan
Copy link
Contributor Author

@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

[BroccoliMergeTrees] error while merging the following:
  1.  [Funnel: Packaged Ember CLI Internal Files]
  2.  [BroccoliMergeTrees: Full Application]
  3.  [BroccoliMergeTrees: TreeMerger (custom transform: amd)]
  4.  [BroccoliMergeTrees: Packaged Tests]
Merge error: conflicting capitalizations:
vendor/moment/locale/en-SG.js in /tmp/broccoli-3490FUwEMm5lCoQl/out-603-broccoli_merge_trees_full_application
vendor/moment/locale/en-sg.js in /tmp/broccoli-3490FUwEMm5lCoQl/out-603-broccoli_merge_trees_full_application
Remove one of the files and re-add it with matching capitalization.

@gitKrystan
Copy link
Contributor Author

@chancancode and I are looking into why canary tests are failing for this PR with TypeError: guidRef.value is not a function. Looks like there is a bug in the Glimmer VM. I am going to do my best to sum it up and he can correct me when he has time. 😝

The AST transform in the ember internals for {{-in-element}} added a guid: https://github.com/emberjs/ember.js/blob/master/packages/ember-template-compiler/lib/plugins/transform-in-element.ts#L105-L107

The transform for {{in-element}} no longer does that. Instead, glimmer-vm is now responsible for adding the guid internally (it requiring a guid is an implementation detail that no one should need to care about). However, that code runs too early. It currently runs before custom AST transforms (like the one in ember-maybe-in-element) are run, so if an AST transform adds another in-element, it will miss the boat on that guid getting added by glimmer.

Currently the flow is:
Parse -> (glimmer add guid argument) -> run AST transforms -> compile into javascript

It should be:
Parse -> run AST transforms -> (glimmer add guid argument) -> compile into javascript

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.

gitKrystan added a commit to tildeio/glimmer-vm that referenced this pull request May 6, 2020
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.
@gitKrystan
Copy link
Contributor Author

gitKrystan commented May 11, 2020

@snewcomer
glimmerjs/glimmer-vm#1086 and emberjs/ember.js#18958 fix this PR on canary.

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
@backspace
Copy link

Sorry for the aside here, @gitKrystan, I’m struggling in CI with the capitalisation conflict you posted about:

[BroccoliMergeTrees] error while merging the following:
  1.  [Funnel: Packaged Ember CLI Internal Files]
  2.  [BroccoliMergeTrees: Full Application]
  3.  [BroccoliMergeTrees: TreeMerger (custom transform: amd)]
  4.  [BroccoliMergeTrees: Packaged Tests]
Merge error: conflicting capitalizations:
vendor/moment/locale/en-SG.js in /tmp/broccoli-3490FUwEMm5lCoQl/out-603-broccoli_merge_trees_full_application
vendor/moment/locale/en-sg.js in /tmp/broccoli-3490FUwEMm5lCoQl/out-603-broccoli_merge_trees_full_application
Remove one of the files and re-add it with matching capitalization.

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 😯

@chancancode
Copy link

@backspace the problem is moment had a bad release in 2.25, see

jasonmit/ember-cli-moment-shim#183 (comment)
miragejs/ember-cli-mirage#1977

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.

@backspace
Copy link

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.

@gitKrystan
Copy link
Contributor Author

Need anything else from me to get this ready for merge?

Copy link
Contributor

@snewcomer snewcomer left a 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.

@snewcomer snewcomer merged commit 5fcd22a into DockYard:master Jun 26, 2020
@gitKrystan gitKrystan deleted the upgrade-polyfill branch June 29, 2020 16:44
@Turbo87
Copy link

Turbo87 commented Jun 30, 2020

We tried to upgrade an app to v1.0 which includes this change and our builds are now failing with:

Assertion Failed: The {{in-element}} helper cannot be used. ('ember-basic-dropdown/templates/components/basic-dropdown/content.hbs' @ L1:C0)

I assume it is related to this PR? 🤔

@cibernox
Copy link
Contributor

@Turbo87 it must be, but not sure why. I'll try to check it this afternoon.

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

Successfully merging this pull request may close these issues.

6 participants