rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

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

Open Alonski opened this issue 5 years ago • 12 comments

Rendered

This was recreated after being closed here: https://github.com/ember-cli/rfcs/pull/100

Alonski avatar Mar 14 '19 07:03 Alonski

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

kellyselden avatar Mar 16 '19 00:03 kellyselden

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.

Gaurav0 avatar Apr 08 '19 16:04 Gaurav0

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.

ef4 avatar Apr 09 '19 14:04 ef4

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.

dgeb avatar Apr 12 '19 16:04 dgeb

@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.

kellyselden avatar Apr 12 '19 16:04 kellyselden

@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.

dgeb avatar Apr 12 '19 16:04 dgeb

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)

kellyselden avatar Apr 12 '19 16:04 kellyselden

@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?

Alonski avatar Apr 13 '19 07:04 Alonski

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?

dgeb avatar Apr 13 '19 12:04 dgeb

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 ❤️

dfreeman avatar Apr 15 '19 12:04 dfreeman

Any updates on this one?

mehulkar avatar Nov 03 '20 04:11 mehulkar

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

wagenet avatar Jul 23 '22 18:07 wagenet

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 avatar Dec 07 '22 18:12 kategengler

FCP to Close has completed

kategengler avatar Dec 14 '22 18:12 kategengler