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 hasSingleImplementation, assertHasSingleImplementation (highlander) #156

Merged
merged 7 commits into from
Jan 17, 2020

Conversation

xg-wang
Copy link
Member

@xg-wang xg-wang commented Dec 24, 2019

I have a use case where the addon has build registry so that it needs to be listed as dependencies in other addons that uses it, but it brings in additional assets.
ember-cli does the "firstWriteWins" strategy, but I need to assert there's no duplication in node_modules that surprises the final build.

Comparing to emberjs/ember-test-waiters#86, this checker does not mutate anything in the hierarchy but rather throws a silent error to force app fixes the resolution.

@xg-wang
Copy link
Member Author

xg-wang commented Dec 24, 2019

Not sure about the lint error, they are passing locally.

@xg-wang
Copy link
Member Author

xg-wang commented Jan 6, 2020

Ran yarn upgrade and fixed the lint error. @rwjblue @stefanpenner need some review

@xg-wang xg-wang changed the title Add assertHighlander, assertSingleton to checker.for() Add isSingleton, assertSingleton to checker.for() Jan 10, 2020
@xg-wang
Copy link
Member Author

xg-wang commented Jan 10, 2020

I cannot think about a case I need isHighlander that's not fixable by isSingleton so removing that.

tests/index.js Outdated Show resolved Hide resolved
src/dependency-version-checker.js Outdated Show resolved Hide resolved
src/discover-addons.js Outdated Show resolved Hide resolved
src/dependency-version-checker.js Outdated Show resolved Hide resolved
src/discover-addons.js Outdated Show resolved Hide resolved
@stefanpenner
Copy link
Collaborator

@xg-wang sorry for the late replies, this looks great. I would like to see us move forward, but let's double check with @rwjblue.

@xg-wang
Copy link
Member Author

xg-wang commented Jan 15, 2020

@stefanpenner thx, agree with your reviews.
I did chat with @rwjblue, the things he was worrying about are

  1. naming, instance is not the addon "instance"
  2. Error message may tell user all addons' root.
    I was suggesting in doc people can use yarn why to get (2) but there're times node_modules aren't reflected from yarn why.

tests/index.js Show resolved Hide resolved
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

new direction looks good overall

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/is-unique-in-project.js Outdated Show resolved Hide resolved
src/project-wide-dependency-checker.js Outdated Show resolved Hide resolved
@stefanpenner stefanpenner force-pushed the highlander branch 5 times, most recently from e1bc141 to 91e9d6d Compare January 16, 2020 23:34
@stefanpenner stefanpenner changed the title Add isSingleton, assertSingleton to checker.for() Add hasSingleImplementation, assertHasSingleImplementation Jan 16, 2020
@stefanpenner stefanpenner force-pushed the highlander branch 3 times, most recently from f93c038 to cdd3a85 Compare January 16, 2020 23:41
src/dependency-version-checker.js Outdated Show resolved Hide resolved
src/project-wide-dependency-checker.js Outdated Show resolved Hide resolved
src/utils/single-implementation.js Outdated Show resolved Hide resolved
@stefanpenner stefanpenner force-pushed the highlander branch 3 times, most recently from 75f5d04 to bb046cf Compare January 17, 2020 00:21
@stefanpenner stefanpenner force-pushed the highlander branch 4 times, most recently from 00df6fe to 711d3a3 Compare January 17, 2020 01:17
This checker is specific to asserting and checking scenarios not against a single level, but rather project wide.

Functionality:

* `VersionChecker.forProject(project).{hasSingleImplementation,assertSingleImplementation}('addon-name'); // => true|false`
* `VersionChecker.forProject(project).filterAddonByName('addon-name'); // => addon[]`
* `VersionChecker.forProject(project).allAddons(); // iterator across
  all addons (dfs)
@stefanpenner stefanpenner changed the title Add hasSingleImplementation, assertHasSingleImplementation Add hasSingleImplementation, assertHasSingleImplementation (highlander) Jan 17, 2020
@stefanpenner stefanpenner merged commit b476a57 into ember-cli:master Jan 17, 2020
@xg-wang xg-wang deleted the highlander branch January 17, 2020 06:30
@stefanpenner stefanpenner mentioned this pull request Feb 14, 2020
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.

None yet

3 participants