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 api to ember 4.12 #1833

Merged
merged 7 commits into from
Aug 14, 2023
Merged

Conversation

ZedLi
Copy link
Collaborator

@ZedLi ZedLi commented Aug 9, 2023

Description

Initial upgrade of ember and ember-data to 4.12 for API addon.

@ZedLi ZedLi requested a review from a team as a code owner August 9, 2023 18:53
@ZedLi ZedLi self-assigned this Aug 9, 2023
@vercel
Copy link

vercel bot commented Aug 9, 2023

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

Name Status Preview Comments Updated (UTC)
boundary-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 11, 2023 9:45pm
boundary-ui-desktop ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 11, 2023 9:45pm
boundary-ui-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 11, 2023 9:45pm

@@ -82,7 +82,8 @@
"**/@storybook/**/ansi-regex": "^5.0.1",
"**/jsprim/json-schema": "^0.4.0",
"**/@storybook/**/yuidocjs/markdown-it": "^12.3.2",
"**/ember-cli/markdown-it-terminal/markdown-it": "^12.3.2"
"**/ember-cli/markdown-it-terminal/markdown-it": "^12.3.2",
"**/ember-cli-mirage/@embroider/macros": "^1.0.0"
Copy link
Collaborator Author

@ZedLi ZedLi Aug 9, 2023

Choose a reason for hiding this comment

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

This resolution is needed because there's a current issue when building with multiple versions of @embroider/macros. See this issue and this one for more info. Until ember-cli-mirage releases the next version that uses a version that's >1.0.0, we'll need to force it.

Copy link
Collaborator

@DhariniJeeva DhariniJeeva left a comment

Choose a reason for hiding this comment

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

LGTM!

left a non-blocking question

@@ -10,67 +10,70 @@
"scripts": {
"build:development": "ember build",
"build": "ember build --environment=production",
"lint": "npm-run-all --aggregate-output --continue-on-error --parallel \"lint:!(fix)\"",
"lint:fix": "npm-run-all --aggregate-output --continue-on-error --parallel lint:*:fix",
"lint": "concurrently \"npm:lint:*(!fix)\" --names \"lint:\"",
Copy link
Collaborator

Choose a reason for hiding this comment

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

None blocking comment: I guess ember-cli deprecated npm-run-all and installed concurrently? or we did so?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah this was all ember-cli 😆

Copy link
Collaborator

@calcaide calcaide left a comment

Choose a reason for hiding this comment

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

LGTM!! Great upgrade!! first addon to pass the 4.0 and to mebrace newer node support!

Copy link
Collaborator

@cameronperera cameronperera left a comment

Choose a reason for hiding this comment

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

Nice work!

const pluginName = snapshot.attr('plugin')?.name;
if (pluginName) url = `${url}?plugin_name=${pluginName}`;
} catch (e) {
// Ignore any adapter errors here
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These try catches are needed now because for some reason, if there's a 404 you get a cache error by just trying to access the snapshot attribute

@@ -124,10 +123,7 @@ module('Acceptance | authentication', function (hooks) {

test('visiting auth method when the scope cannot be loaded is still allowed', async function (assert) {
assert.expect(1);
setupOnerror(() => {
Copy link
Collaborator Author

@ZedLi ZedLi Aug 11, 2023

Choose a reason for hiding this comment

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

Seems like this doesn't work anymore, possibly because ember changed what uses ember runloop in the most recent changes and this only catches things on the runloop.

Either way it doesn't appear it's needed in the first place since we weren't mocking the correct route anyways

@ZedLi ZedLi merged commit dabdb9a into search-and-filtering-llb Aug 14, 2023
@ZedLi ZedLi deleted the upgrade-api-to-ember-4.12 branch August 14, 2023 16:37
ZedLi added a commit that referenced this pull request Aug 14, 2023
* chore: 🤖 Upgrade API layer to ember 4.12

* chore: 🤖 Upgrade ember-data to 4.12

* fix: 🐛 Fix breaking change from usage of internal method

* chore: 🤖 Remove stylelint in API addon

* test: 💍 Fix broken API tests

* chore: 🤖 get rid of toArray

* test: 💍 Fix tests broken by ember data changes

---------

Co-authored-by: dharini <[email protected]>
ZedLi added a commit that referenced this pull request Aug 14, 2023
* chore: 🤖 Upgrade API layer to ember 4.12

* chore: 🤖 Upgrade ember-data to 4.12

* fix: 🐛 Fix breaking change from usage of internal method

* chore: 🤖 Remove stylelint in API addon

* test: 💍 Fix broken API tests

* chore: 🤖 get rid of toArray

* test: 💍 Fix tests broken by ember data changes

---------

Co-authored-by: dharini <[email protected]>
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.

4 participants