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

Replace getWithDefault (Fixes #1970) #1971

Merged
merged 1 commit into from
Jun 8, 2020

Conversation

gitKrystan
Copy link
Contributor

@gitKrystan gitKrystan commented May 29, 2020

getWithDefault is deprecated by emberjs/rfcs#554, causing failures with Ember Canary. See #1970

@gitKrystan gitKrystan force-pushed the ember-rfc-0554-deprecation branch 2 times, most recently from 39fc6d2 to 3ee1fef Compare May 29, 2020 23:59
@samselikoff
Copy link
Collaborator

Amazing – thank you!

@gitKrystan
Copy link
Contributor Author

@samselikoff Regarding the test failure, I believe it's unrelated. There is a version of moment with two copies of the en-sg.js file: moment/moment@36a817d#diff-fb216d9e8791e63c8d12bdc420956839
We fixed this issue in our app by resolving to a newer (fixed) version of moment. If you know how you want to handle it, I'm happy to open another PR to fix.

@samselikoff
Copy link
Collaborator

Yes – sorry I haven't gotten to this yet. It looks like a problem with the floating dependencies test job. Usually this means there's a change to a transitive dependency somewhere that's broken something.

In the past I've fixed this by manually doing a binomial search on the yarn.lock file... it's kind of ridiculous. Basically, you should be able to reproduce the failure if you run the suite locally with yarn install --no-lockfile, and if you do, the problem is somewhere in there. The next step is to literally delete half of the lock file, run yarn install, and see if you have the error. If you do, the problem is in one of the remaining lines; if not, it was in one of the lines you deleted.

Continue trimming down by half each time until you identify the problematic line of the lockfile. At that point you should have identified the problematic dependency and we can decide whether to just lock it down using Yarn resolutions, or try to help fix the underlying bug.

LMK if that makes sense, and if you get around to it that would be extremely helpful! If not I'll probably get to it this weekend. After we solve it, we can open a PR (separately) with an updated yarn.lockfile, including whatever fix we applied, at which point we can re-run this PR and it should pass.

@chancancode
Copy link
Collaborator

chancancode commented Jun 4, 2020

@samselikoff (or anyone that has time) TL;DR – jasonmit/ember-cli-moment-shim#183 (comment) (I think once you have used the resolution to update the lockfile, it may not be needed anymore)

@gitKrystan
Copy link
Contributor Author

These tests should pass with #1977

@chancancode
Copy link
Collaborator

Rebased against mater to kick off another CI run

@chancancode chancancode merged commit f48e465 into miragejs:master Jun 8, 2020
@chancancode
Copy link
Collaborator

@samselikoff mind cutting a release when you get a chance? not sure what your release process is here

@samselikoff
Copy link
Collaborator

Absolutely - thanks so much for taking care of these over the weekend!

@chancancode
Copy link
Collaborator

No problem! And thanks for the release. I left most of the dep upgrades because they are either more significant or because they require dropping the EOL node versions here, which requires bumping major.

@samselikoff
Copy link
Collaborator

@chancancode gotcha... do all dep upgrades that EOL node require e-c-mirage to bump major? I think this one might have already gotten in: #1897

But if e-c-mirage is not using broccoli-funnel in a way that breaks on older nodes, it's technically not breaking for us, is it?

@chancancode
Copy link
Collaborator

I think if you are running node 8, npm install (or yarn) will fail, because it will fail to find a version of broccoli-funnel that matches all the constraints – within the version range e-c-mirage asked for and supports node 8 according to their package.json, so I think it's generally best practice to consider dropping node versions a breaking change. If this breaks people code, we can revert the bump and re-release 1.1.8. cc @rwjblue

@samselikoff
Copy link
Collaborator

I see. Happy to do that, def. don't mind reverting + bumping versions in the future.

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

Successfully merging this pull request may close these issues.

3 participants