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

only check devDependencies when checking requested range of an app package #629

Merged
merged 3 commits into from
Jun 24, 2024

Conversation

mansona
Copy link
Member

@mansona mansona commented Jun 22, 2024

I noticed this when I was linking a package that had a peerDependency range wider than it's devDependencies (ember-showdown-prism)

i don't understand why we would ever want to check the requested range of a dev depdency of a package 🤔 but perhaps there is a nuance that I don't understand here and we need to encode that nuance into the question that is being asked

mansona added 2 commits June 22, 2024 10:27
I noticed this when I was linking a package that had a peerDependency range wider than it's devDependencies (ember-showdown-prism)

i don't understand why we would ever want to check the requested range of a dev depdency of a package 🤔 but perhaps there is a nuance that I don't understand here and we need to encode that nuance into the question that is being asked
@mansona
Copy link
Member Author

mansona commented Jun 22, 2024

Turns out fixing a lint error is very tough on the mobile app 🤣 I am very proud of my achievement 💪

@ef4
Copy link
Collaborator

ef4 commented Jun 23, 2024

I think this code applies to apps too though. Which need their devDependencies.

We also have hasNonDevDepDependency() in this same class. Code like assertAllowedDependency is sensitive to the difference between apps and addons. If you found a spot that isn't, that is where the bug should be fixed.

@mansona
Copy link
Member Author

mansona commented Jun 24, 2024

I started trying to write down some context for this change and then I figured it would be better to just fix it rather than writing it down 😂 the requestedRange function is only used in one place so it was quite simple to add the new arg to it 👍

I'm pretty sure this fixes my issue but I'm wondering if you could help me place a test for this (unless you think it's not necessary?) 🤔

@mansona mansona force-pushed the fix-requested-range branch from efa57b4 to ee0cb68 Compare June 24, 2024 14:55
@mansona mansona requested a review from ef4 June 24, 2024 15:49
@mansona mansona changed the title stop checking devDependencies when checking requested range of a package only check devDependencies when checking requested range of an app package Jun 24, 2024
@mansona mansona added the bug Something isn't working label Jun 24, 2024
@mansona mansona force-pushed the fix-requested-range branch from 69f7897 to ee0cb68 Compare June 24, 2024 16:04
@ef4 ef4 merged commit 4e3a5aa into main Jun 24, 2024
315 checks passed
@ef4 ef4 deleted the fix-requested-range branch June 24, 2024 18:11
@github-actions github-actions bot mentioned this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants