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

refactor: 💡 Upgrade Ember to 4.4 LTS within addons/api #1490

Merged
merged 16 commits into from
Dec 6, 2022

Conversation

calcaide
Copy link
Collaborator

@calcaide calcaide commented Nov 21, 2022

Description

Be aware:

  • This merges to refactor-upgrade-ember-4 long living branch.

@calcaide calcaide self-assigned this Nov 21, 2022
@calcaide calcaide requested a review from a team as a code owner November 21, 2022 21:52
@vercel
Copy link

vercel bot commented Nov 21, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
boundary-ui ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Dec 6, 2022 at 10:58PM (UTC)
boundary-ui-desktop ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Dec 6, 2022 at 10:58PM (UTC)
boundary-ui-storybook ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Dec 6, 2022 at 10:58PM (UTC)

@@ -0,0 +1,76 @@
name: CI
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the purpose of this CI file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! forgot to delete that one.

Context: this file is included by ember cli when you run the upgrade script. I did not went really deep on this, but when I investigate it, basically they check what CI you use, GH workflows in our case, and adds this file to it.
Supposetly, after adding this file, the CI will run this workflow and basically is running ember-try within the CI.

* refactor: 💡 Refactor Mirage server configuration

Co-authored-by: Zhihe Li <zhihe.li@hashicorp.com>
assert.true(model.hasDirtyAttributes);
assert.true(model.canSave);
assert.false(model.cannotSave);
// Should not be able to save while currently saving
model.transitionTo('updated.inFlight');
model.save().then(() => {
Copy link

Choose a reason for hiding this comment

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

This should probably be awaited, since it's async.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@randallmorey this is the part I cherry pick temporarly (until we merge it) from the PR that fixes ember-data issue, here the link. We might want to fix this there 😉

Copy link

Choose a reason for hiding this comment

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

Yes

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually don't want to await it. This is so I can assert the isSaving transition and then once the promise is resolved assert that it transitioned correctly.

Copy link

Choose a reason for hiding this comment

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

Ah got it. There may be a way to express this in a way that accomplishes both goals.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup I couldn't figure out a better way to do it, happy to change it if we can think of a better solution

Copy link

Choose a reason for hiding this comment

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

Made a suggestion. I wanted to make the await explicit for readability. While it probably works without await, that's because Ember tests automagically await the runloop. It's perfectly valid, but might be confusing for anyone new to Ember.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good, made the change in the other PR

@calcaide calcaide requested a review from a user December 6, 2022 19:00
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.

3 participants