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

Add @ember/string as a dependency #139

Merged
merged 2 commits into from
Jan 16, 2023
Merged

Add @ember/string as a dependency #139

merged 2 commits into from
Jan 16, 2023

Conversation

boris-petrov
Copy link
Contributor

@boris-petrov boris-petrov commented Jan 13, 2023

To fix Ember 4.10 deprecation warnings.

Similar to ember-cli/ember-resolver#862.

cc @jelhan

@jelhan jelhan added enhancement New feature or request bug Something isn't working and removed enhancement New feature or request labels Jan 13, 2023
@jelhan
Copy link
Owner

jelhan commented Jan 13, 2023

@boris-petrov CI is failing:

ember-style-modifier tried to import "@ember/string" in "ember-style-modifier/modifiers/style.js" but the package was not resolvable from /home/runner/work/ember-style-modifier/ember-style-modifier

Maybe you need to run yarn install to upgrade the lock file?

@boris-petrov
Copy link
Contributor Author

I did yarn install, however Yarn doesn't install peer deps. So I guess that's the problem. I'll add it to devDependencies?

@jelhan
Copy link
Owner

jelhan commented Jan 13, 2023

I did yarn install, however Yarn doesn't install peer deps. So I guess that's the problem. I'll add it to devDependencies?

If it's a package, which needs to be available in consuming apps, I think it needs to go into dependencies.

@boris-petrov
Copy link
Contributor Author

If it's a package, which needs to be available in consuming apps, I think it needs to go into dependencies.

I'm really not sure. I just did what the Ember team did in ember-resolver. I'm fine moving it to dependencies if you prefer though?

@jelhan
Copy link
Owner

jelhan commented Jan 13, 2023

If it's a different package used at run-time it needs to go into dependencies similar as ember-modifier. At least if there isn't something special about that package.

To fix Ember 4.10 deprecation warnings
@boris-petrov
Copy link
Contributor Author

Done, I added it only in dependencies.

@boris-petrov boris-petrov changed the title Add @ember/string as a peer-dependency Add @ember/string as a dependency Jan 13, 2023
@boris-petrov
Copy link
Contributor Author

@jelhan I asked in the Ember Discord here what should we do so let's wait and see.

@jelhan
Copy link
Owner

jelhan commented Jan 13, 2023

@jelhan I asked in the Ember Discord here what should we do so let's wait and see.

Thanks a lot for raising the question on Discord. Let's wait until we get a clear path forward for addons. Would like to avoid unnecessary churn.

@jelhan
Copy link
Owner

jelhan commented Jan 14, 2023

Thanks a lot for starting the discussion on Discord. Getting some recommendations from other community members and core team about recommended path forward was helpful to me.

Adding @ember/string as a peer dependency seems to be the recommended path forward. This is a breaking change.

In v1 addon it needs to be a dev dependency as well as it is needed for the build-in test app. That will resolve the error, which we noticed on the first attempt #139 (comment).

I'm not a big fan of having another major. I feel ember-style-modifier had too many majors in the last months. The last major release (v2.0.0) is less than one month ago. We had another major release (v1.0.0) in October. Many addons depending on ember-style-modifier haven't caught up with any of them yet. Another major does not help. But diverging from best practices in Ember ecosystem won't help either.

I had a quick look if we could drop usage of @ember/string at all. It is used to dasherize strings. While many other packages exists, which supports that as well, I feel there is value in having a standard across Ember ecosystem. It helps keeping bundle size low.

Would be great if you could upgrade this PR accordingly. I fully understand if you don't have the time. In that case please let me know and I can take over.

@jelhan jelhan added breaking and removed bug Something isn't working labels Jan 14, 2023
@boris-petrov
Copy link
Contributor Author

boris-petrov commented Jan 16, 2023

@jelhan sorry, in the end I didn't quite understand what you want to happen. 😄 I've pushed a second commit that adds @ember/string as both a devDependency and peerDependency. Is that what you wanted? If so, I (or even you) can squash the commits. But FWIW, I believe that the right way (for now) is to add it in dependencies. That way you can skip the major release for now. And you can always move it to peerDependencies later and do a breaking change. Best of both worlds.

@jelhan
Copy link
Owner

jelhan commented Jan 16, 2023

I've pushed a second commit that adds @ember/string as both a devDependency and peerDependency. Is that what you wanted?

Yes. That seems to be the recommended path forward.

But FWIW, I believe that the right way (for now) is to add it in dependencies. That way you can skip the major release for now. And you can always move it to peerDependencies later and do a breaking change.

I would prefer going with the recommended path forward even if that means having another major release. Adding @ember/string as a dependency could also cause some troubles for consumers. E.g. it may cause an application to include two different versions of @ember/string in production build. Having that many major releases is not great. But I feel it is the better trade-off.

Copy link
Owner

@jelhan jelhan left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this one!

@jelhan jelhan merged commit 0e3e9a9 into jelhan:master Jan 16, 2023
@jelhan
Copy link
Owner

jelhan commented Jan 16, 2023

Published as v3.0.0.

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

Successfully merging this pull request may close these issues.

2 participants