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 eslint-plugin-import to ember-cli blueprint #894

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bmish
Copy link
Contributor

@bmish bmish commented Jan 7, 2023

This RFC proposes adding eslint-plugin-import to the ember-cli blueprint.

I talked to @wycats about this 2 years ago, and he said this was the "most important linter" to add, but mentioned there were some issues that would need to be solved with it. I assume he was referring to the resolver issues, which we are bypassing for now by disabling the import/no-unresolved lint rule.

@github-actions github-actions bot added the S-Proposed In the Proposed Stage label Jan 7, 2023
@bmish bmish force-pushed the eslint-plugin-import branch 3 times, most recently from 62b81b0 to 01c0fab Compare January 7, 2023 19:03

### Resolver

There are a few issues to fix with the [import/no-unresolved](https://github.com/import-js/eslint-plugin-import/blob/HEAD/docs/rules/no-unresolved.md) rule.
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Did some testing on this plugin today in my own https://github.com/NullVoxPopuli/eslint-configs

import/no-unresolved does not understand package.json#exports -- which is a bit of a deal breaker for modern packages.

I tested with vitest:

eslint-configs/vitest.config.ts
  1:30  error  Unable to resolve path to module 'vitest/config'  import/no-unresolved
# ...
❯ cat node_modules/vitest/package.json | jq '.exports';
{
  ... ✂️...
  "./config": {
    "types": "./config.d.ts",
    "require": "./dist/config.cjs",
    "import": "./dist/config.js"
  }
  

So far, all V2 addons also use package.json#exports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for testing that. I have updated the RFC to disable the import/no-unresolved lint rule.

@ef4
Copy link
Contributor

ef4 commented Mar 1, 2023

I think it's probably not possible to achieve reliable detection of unresolvable imports in traditional apps and addons. People are allowed to emit arbitrary AMD modules from a whole bunch of possible places in the build, across all your addons and their addons, and those are all imports that work at runtime but are totally opaque to a static linter.

In embroider we are stabilizing around having one standardized module resolver that would potentially be able to power a lint rule, but even there when we're looking at a v1 addon or v1 app (and at this point all apps are v1 apps because we haven't done a v2 app format RFC), we never emit resolver failures, because when we run out of possible static places to look we need to assume there might be a runtime answer, so we emit a runtime lookup.

V2 addons do follow strict enough rules that an unresolvable import lint rule can work reliably there. The list of traditional ember-source provided imports is known and hard-coded, and everything else is required to be statically resolvable.

It's possible we'd want to be similarly strict in a v2 app format, as long as we can explain what the upgrade path is such that apps can upgrade even if not every addon has done so.

This RFC is yet another example of something that would be easier to do if we can eliminate the AMD loader.

@NullVoxPopuli
Copy link
Sponsor Contributor

I'm good with disabling import/no-unresolved 💯 the lint that has helped me out the most is import/no-extraneous-dependencies (or n/no-extraneous-imports) -- it helps folks properly manage their package.json, which can get very messed up in npm and yarn projects.

I'd even be a fan of splitting up the RFC per lint rule, if needed, because the plugin clearly has some good rules that we can use today and can help guide folks towards better configured projects -- all without waiting on v2 stuff

bmish added 2 commits May 20, 2023 21:23
* master: (56 commits)
  Fix code examples & add ember-cli release version in emberjs#637
  Update FCP guidance to include Discord
  Update RFC 085421 ready-for-release PR URL
  Advance RFC {{ inputs.rfc-number }} to Stage ready-for-release
  feat: EmberData Cache v2.1
  finalize lifetimes
  Update text/0860-ember-data-request-service.md
  Update RFC 496, typos, correct field name
  add note
  chore: update RequestService url with finalized design details
  Move emberjs#331 deprecate-globals-resolver to recommended
  Correct metadata for emberjs#487 custom model classes
  Move emberjs#625 helper-managers to recommended
  Add release date and version for 776
  Update RFC 0776 released PR URL
  Advance RFC {{ inputs.rfc-number }} to Stage released
  Update RFC 0739 ready-for-release PR URL
  Advance RFC {{ inputs.rfc-number }} to Stage ready-for-release
  Deprecate `ember-mocha`
  Add title of RFC to advancement PR titles
  ...
@bmish
Copy link
Contributor Author

bmish commented May 21, 2023

@ef4 thanks for the context. I have updated the RFC to disable the import/no-unresolved lint rule to get around the current infeasibility of reliably resolving imports. We can always come back to it later if it becomes possible, but this unblocks getting the rest of eslint-plugin-import into place.

@wagenet wagenet added S-Exploring In the Exploring RFC Stage and removed S-Proposed In the Proposed Stage labels May 26, 2023
@wagenet
Copy link
Member

wagenet commented May 26, 2023

We've moved this to Exploring.

@ef4
Copy link
Contributor

ef4 commented Aug 18, 2023

I still don't think we can safely implement this until after something like #938 because there's too much stuff that is valid and working but not detectable as a real dependency, even beyond just the standard ember-provided ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Champion S-Exploring In the Exploring RFC Stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants