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.

Sign up for GitHub

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

Jump to bottom

Update manager capabilities to 3.22 to fix deprecation in Ember 3.26 #33

Closed
wants to merge 12 commits into from
Closed

Update manager capabilities to 3.22 to fix deprecation in Ember 3.26 #33

wants to merge 12 commits into from

Conversation

cibernox
Copy link

@cibernox cibernox commented Mar 5, 2021 •
edited
Loading

This is flooding my logs with thousands of warnings when using ember beta (currently 3.26).

rwjblue
rwjblue reviewed Mar 5, 2021
View reviewed changes
addon/modifiers/did-insert.js Outdated
@@ -46,7 +46,7 @@ import { setModifierManager, capabilities } from '@ember/modifier';
*/
export default setModifierManager(
() => ({
capabilities: capabilities('3.13', { disableAutoTracking: true }),
capabilities: capabilities('3.22', { disableAutoTracking: true }),
Copy link
Member

Choose a reason for hiding this comment

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

Needs to use gte to use the capabilities as available by Ember version

See #25 (review)

@sandydoo sandydoo mentioned this pull request Apr 23, 2021
Closed
@simonihmig
Copy link

This is flooding my logs with thousands of warnings

Same here :) @cibernox would you be able to address the PR review here?

@cibernox
Copy link
Author

I missed the feedback completely on this one. I just fixed it.

@cibernox cibernox force-pushed the update-manager-capabilities-to-3-22 branch from 3af6ba6 to d3d4d16 Compare May 20, 2021 21:53
@cibernox cibernox force-pushed the update-manager-capabilities-to-3-22 branch from 53cefc8 to 6df898b Compare May 20, 2021 22:44
@cibernox
Copy link
Author

Not sure why did-update is failing for ember 3.24+

@sandydoo
Copy link

@cibernox, probably related to autotracking. did-update doesn’t “consume” the arguments on initial render, so the update hook isn’t run. The hack-du-jour is to loop over all of the arguments, like in this previous attempt to solve this same issue #25 (comment).
It looks like the Ember team decided to actually use the workaround in the meantime (ember-modifier/ember-modifier#63 (comment)), but things have stalled…

@boris-petrov
Copy link

That's also what we did in ember-render-helpers.

@simonihmig simonihmig mentioned this pull request May 25, 2021
Merged
@alexlafroscia alexlafroscia mentioned this pull request Jun 16, 2021
Closed
@jgadbois
Copy link

@simonihmig Is there anything remaining to get this merged?

@simonihmig
Copy link

@simonihmig Is there anything remaining to get this merged?

Well, I am neither the author nor one of the maintainers, so it's not really up to me! 🙂
But AFAICT it's just that one test scenario that is still failing...

@jgadbois
Copy link

@simonihmig Is there anything remaining to get this merged?

Well, I am neither the author nor one of the maintainers, so it's not really up to me!
But AFAICT it's just that one test scenario that is still failing...

Whoops! I'll see if i can look at that test scenario later. The deprecation warnings are getting pretty intense :)

@cibernox cibernox force-pushed the update-manager-capabilities-to-3-22 branch from eed06eb to e3574ed Compare June 18, 2021 16:41
@cibernox
Copy link
Author

@rwjblue I had to pass disableAutoTracking: false (check 7146c3a ) to make tests pass in 3.24.

Tests continue to hang forever in release / beta / canary without anything being logged. I recall IE11 being to blame on this, but I removed it from the targets and it's still hanging. Any idea what might be causing it?

@jgadbois
Copy link

Hi @cibernox thanks for working on this - have you made any progress on the remaining tests failing?

@btecu
Copy link

btecu commented Jul 25, 2021

@cibernox why the switch from yarn to npm?
Was it necessary to drop IE support? That will likely require a major release.

RobbieTheWagner
RobbieTheWagner reviewed Aug 26, 2021
View reviewed changes
.github/workflows/ci-build.yml
@@ -20,13 +20,13 @@ jobs:
with:
node-version: 10.x
- name: install dependencies
run: yarn install
run: npm install
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should need to switch from yarn to npm here, right?

@RobbieTheWagner
Copy link
Member

@cibernox @rwjblue what do we need to do to get this merged in?

@RobbieTheWagner
Copy link
Member

@cibernox @rwjblue bump, this is really necessary for all modern Ember versions.

@cibernox
Copy link
Author

@rwwagner90 I'm a bit short of time right now to push thing forward, maybe someone can take over where I left? Tests were hanging forever and I don't recall the reason (or even if I knew the reason at some point)

This was referenced Sep 19, 2021
Closed
Merged
@simonihmig simonihmig mentioned this pull request Sep 24, 2021
Closed
This was referenced Sep 29, 2021
Closed
Merged
@SergeAstapov
Copy link
Contributor

I re-created this as #42 which needs to land after #40 in order to get CI happy (see #42 description for more info about CI failures).

@jgadbois
Copy link

Awesome, thank you so much!

@RobbieTheWagner RobbieTheWagner added the duplicate This issue or pull request already exists label Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandydoo sandydoo sandydoo left review comments

@RobbieTheWagner RobbieTheWagner RobbieTheWagner left review comments

@rwjblue rwjblue rwjblue left review comments

Assignees
No one assigned
Labels
duplicate This issue or pull request already exists
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@cibernox @simonihmig @sandydoo @boris-petrov @jgadbois @btecu @RobbieTheWagner @SergeAstapov @rwjblue