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

Fix @ember/string dependency #1582

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

anehx
Copy link
Contributor

@anehx anehx commented Jul 30, 2024

@ember/string must be a dependency as ember-cli-addon-docs imports it in their codebase. Currently this still works, but with v4 of @ember/string (v2 addon) this will break as it fully relies on ember-auto-import.

@RobbieTheWagner
Copy link
Member

@anehx should it be a peerDep?

@anehx
Copy link
Contributor Author

anehx commented Aug 6, 2024

@RobbieTheWagner I don't think so. This addon is mostly used in other addons which do not include @ember/string in the devDependencies in their blueprint so tthey would have to install the package manually..

@RobbieTheWagner
Copy link
Member

I think we should probably make it a peerDep and allow v3 and v4 like this https://github.com/minutebase/ember-can/blob/master/ember-can/package.json#L129

@anehx anehx force-pushed the fix-string-dependency branch from 2ea5da3 to a0aa434 Compare August 7, 2024 06:53
@anehx
Copy link
Contributor Author

anehx commented Aug 7, 2024

@RobbieTheWagner Fair enough. I changed it in this PR.

package.json Outdated
@@ -105,7 +105,6 @@
"@babel/plugin-proposal-decorators": "^7.23.6",
"@babel/preset-env": "^7.23.6",
"@ember/optional-features": "^2.0.0",
"@ember/string": "^3.1.1",
Copy link
Member

Choose a reason for hiding this comment

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

This will still need to be a devDep too right, so we're installing it ourselves when running the test app etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, my bad. Fixed!

`@ember/string` must be a peer dpendency as `ember-cli-addon-docs` imports
it in their codebase. Currently this still works, but with v4 of
`@ember/string` (v2 addon) this will break as it fully relies on
`ember-auto-import`.
@anehx anehx force-pushed the fix-string-dependency branch from a0aa434 to e37a134 Compare August 7, 2024 12:09
@RobbieTheWagner
Copy link
Member

CI is failing for other reasons, so I will go ahead and merge this. Thanks for the PR!

@RobbieTheWagner RobbieTheWagner merged commit fd6c6b2 into ember-learn:master Aug 7, 2024
7 of 15 checks passed
@anehx anehx deleted the fix-string-dependency branch August 7, 2024 14:49
@ijlee2
Copy link
Member

ijlee2 commented Sep 9, 2024

Hi, @RobbieTheWagner. Could you help release a patch version of ember-cli-addon-docs?

The documentation site for ember-intl needs the fix in this PR to be able to update @ember/string to v4. A new version would also help me remove a custom patch that I had upstreamed in #1556.

@RobbieTheWagner
Copy link
Member

@ijlee2 I would love to help release a new version, but CI is failing currently. If you have time to diagnose and fix those failures, we could definitely get a new version out.

@ijlee2
Copy link
Member

ijlee2 commented Sep 9, 2024

@RobbieTheWagner Ah, I didn't see the CI failing in the main branch.

Since the error message refers to @babel/plugin-transform-class-static-block, I'd try updating Babel-related dependencies to the latest version. This may require additional maintenance work, though. Maybe we can look into it after EmberFest.

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