DockYard / ember-maybe-in-element

Conditionally render content elsewhere using #-in-element on ember apps
MIT License
15 stars 10 forks source link

Upgrade ember-in-element-polyfill #25

Closed gitKrystan closed 4 years ago

gitKrystan commented 4 years ago

Fixes deprecation warnings on Ember canary:

The use of the private `{{-in-element}}` is deprecated, please refactor to the public `{{in-element}}`.

See https://github.com/kaliber5/ember-in-element-polyfill/pull/81

gitKrystan commented 4 years ago

I pulled https://github.com/DockYard/ember-maybe-in-element/pull/26 into this PR to get CI running, but we can always drop that commit once #26 is merged.

gitKrystan commented 4 years ago

This doesn't work IRL because of https://github.com/DockYard/ember-maybe-in-element/issues/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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 https://github.com/DockYard/ember-maybe-in-element/issues/10#issuecomment-622055376 is accepted, ember-maybe-in-element would not be affected by the glimmer issue at all.

gitKrystan commented 4 years ago

@snewcomer https://github.com/glimmerjs/glimmer-vm/pull/1086 and https://github.com/emberjs/ember.js/pull/18958 fix this PR on canary.

backspace commented 4 years ago

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 commented 4 years ago

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

https://github.com/jasonmit/ember-cli-moment-shim/issues/183#issuecomment-627193599 https://github.com/miragejs/ember-cli-mirage/pull/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 commented 4 years ago

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 commented 4 years ago

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

Turbo87 commented 4 years ago

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 commented 4 years ago

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