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

Include ember-cli-dependency-lint in the default app blueprint [Revived] #464

Closed
wants to merge 1 commit into from

Conversation

Alonski
Copy link
Member

@Alonski Alonski commented Mar 14, 2019

Rendered

This was recreated after being closed here:
ember-cli/rfcs#100

@kellyselden
Copy link
Member

Seems good to me. If Salsify is OK with increasing the bus factor by giving some of us permission.

@Gaurav0
Copy link
Contributor

Gaurav0 commented Apr 8, 2019

My thinking is we could consider adding a test (not sure which repo) that generates a default ember-cli stable and beta app, and installs the top 10-20 non-default addons and check regularly that there are no flagged addons. This could greatly reduced the number of issues beginner users might run into.

@ef4
Copy link
Contributor

ef4 commented Apr 9, 2019

I am in favor of this lint but it is critical that we give people clear, actionable feedback on how to fix it. One solution that isn't mentioned in the README is yarn resolution, which (as long as you're using yarn) is often ideal.

Dealing with duplicates is important for embroider, because it more faithfully follows node's behavior, meaning you truly can get two versions of an addon in your app, unless you take steps to flatten things down.

There are known cases (like having a duplicated ember-inflector) where switching to embroider gives you a hard error until you deal with the duplication, because the addon in question crashes if loaded more than once.

@dgeb
Copy link
Member

dgeb commented Apr 12, 2019

I'm working on a fork of ember-cli-dependency-lint called ember-cli-addon-guard. v0.1.1 was just released, so feel free to try it out.

I created ember-cli-addon-guard because I wanted to experiment with several factors at once without introducing a bunch of breaking changes to the original. Some differences include:

  • By default, ember-cli-addon-guard runs prior to every build and strictly prevents addon dependency conflicts that have not been explicitly ignored.

  • ember-cli-addon-guard is only concerned with addons that introduce run-time modules. This is inferred from the directories and files present in each addon. Build-time-only addons don't need to be explicitly safelisted, as they do in ember-cli-dependency-lint.

  • ember-cli-addon-guard identifies addons uniquely by name and cache-key, if available, for compatibility with the current approach taken by ember-cli. If cache-keys are not supported by a particular addon, version will be used, which should be forward-compatible with embroider.

  • ember-cli-addon-guard has a unique configuration file with unique settings (so it doesn't conflict / override those for ember-cli-dependency-lint).

  • It's been converted to TypeScript (but of course is transpiled and shipped as JS).

  • I'm experimenting with the possibility of allowing duplicate versions of individual addons to coexist in the same project safely.

I discussed this with @dfreeman (who created ember-cli-dependency-lint) prior to starting, and will be glad to contribute it back to Salsify or to the ember-cli org - wherever it can be most useful.

My hope is to arrive at a single shared solution that ends up in the default app blueprint, in the spirit of this RFC. I think this is badly needed to avoid some common footguns.

@kellyselden
Copy link
Member

@dgeb Do you think it makes sense to incorporate the ember-cli-dependency-checker functionality too? Of course it would no longer be addon-focused.

@dgeb
Copy link
Member

dgeb commented Apr 12, 2019

@kellyselden it very well might make sense to incorporate a subset of the ember-cli-dependency-checker functionality, so we can support a single addon that checks dependencies, addons or not. (I say a subset because there's quite a bit in the current ember-cli-dependency-checker that's no longer needed in modern ember apps: bower, shrinkwrap, etc.)

I'd be open to a rename if there's consensus that this would be a good idea. At that point, I think adopting this in the ember-cli org would make a lot of sense.

@kellyselden
Copy link
Member

In theory they have different goals, but in terms of new apps, seeing two addons that deal with dependency linting might seem offputting? (especially for newcomers)

@Alonski
Copy link
Member Author

Alonski commented Apr 13, 2019

@dgeb @kellyselden Do you think it makes sense to add what you are both talking about to the RFC so that it aligns better with the future plans?

@dgeb
Copy link
Member

dgeb commented Apr 13, 2019

Do you think it makes sense to add what you are both talking about to the RFC so that it aligns better with the future plans?

@Alonski Definitely, once those future plans come into slightly better focus.

Some things that need more discussion:

  • Does ember-cli-addon-guard actually meet everyone's needs? It's very new and needs review.
  • Should the still-relevant capabilities of ember-cli-dependency-checker be rolled back into this addon? If so, should ember-cli-addon-guard be renamed?
  • While iterating over addons and other dependencies, are there additional checks that should take place (e.g. strong validations of peerDependencies, something I was just discussing with @mansona)? Can these checks be better served by 3rd party packages?
  • Are additional considerations required to maintain compatibility with embroider v1 and v2 packages?

@dfreeman
Copy link
Contributor

Assuming we can get @dgeb's package to the point where it's a standard across the ecosystem (which seems likely), I'd be happy to see it live in the ember-cli org and deprecate salsify/ember-cli-dependency-lint in its favor ❤️

@mehulkar
Copy link
Contributor

mehulkar commented Nov 3, 2020

Any updates on this one?

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

@dgeb what's the status on your work here?

@wagenet wagenet added the S-Proposed In the Proposed Stage label Dec 2, 2022
@kategengler
Copy link
Member

Under embroider, two addons can use two versions of a dependency and things won't break. In light of this, we think this addon should still be optional going forward. We're moving this to FCP to close.

If the community do still want to consider adding this to the blueprint in the future, the addon should be updated to no longer lint in tests (as other lints have moved away from this as well). The command to lint could then be added to the "lint" command in package.json

@kategengler kategengler added the FCP to close The core-team that owns this RFC has moved that this RFC be closed. label Dec 7, 2022
@kategengler
Copy link
Member

FCP to Close has completed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FCP to close The core-team that owns this RFC has moved that this RFC be closed. S-Proposed In the Proposed Stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants