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

fix for eslint rule protecting against importing packages that aren't in the package.json #8631

Closed
wants to merge 17 commits into from

Conversation

alshdavid
Copy link
Contributor

↪️ Pull Request

This PR corrects the misbehaving eslint-plugin-import/no-extraneous-dependencies rule, adding protection against using imports that are not defined in a package's package.json

Note:
This currently excludes type imports like import type { Foo } from 'foo' due to the current version of eslint-plugin-import not including the change which adds support for type imports.

I am not sure when they will merge this, the last update was almost a year ago.

💻 Examples

import('foo')

// Will fail to lint unless the closest "package.json" has "foo"
// in either the dependencies or devDependencies

🚨 Test instructions

As this is a linting rule, to test it you can try importing a package that isn't in that package.json to see if it fails

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@alshdavid alshdavid force-pushed the alsh/fixing-eslint-dependency-rule branch 2 times, most recently from a340ca8 to 08613c9 Compare November 14, 2022 06:48
@parcel-benchmark
Copy link

parcel-benchmark commented Nov 14, 2022

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.45s +28.00ms
Cached 319.00ms +13.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/legacy/index.html 826.00b -3.00b 🚀 392.00ms -5.00ms

Cached Bundles

Bundle Size Difference Time Difference
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 233.00ms +19.00ms ⚠️
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 234.00ms +20.00ms ⚠️
dist/modern/parcel.7cdb0fad.webp 102.94kb +0.00b 233.00ms +19.00ms ⚠️
dist/legacy/index.html 826.00b -3.00b 🚀 417.00ms +10.00ms
dist/legacy/index.b8ae99ba.css 94.00b +0.00b 244.00ms +22.00ms ⚠️
dist/modern/index.31cedca9.css 94.00b +0.00b 243.00ms +22.00ms ⚠️

React HackerNews ✅

Timings

Description Time Difference
Cold 6.81s +75.00ms
Cached 410.00ms -7.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/PermalinkedComment.60e78a07.js 4.18kb +0.00b 400.00ms -27.00ms 🚀
dist/UserProfile.c18819ee.js 1.57kb +0.00b 400.00ms -27.00ms 🚀
dist/NotFound.cfeedbab.js 427.00b +0.00b 400.00ms -27.00ms 🚀

Cached Bundles

No bundle changes detected.

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 59.11s -1.27s
Cached 1.99s +94.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/media-viewer.bd165005.js 542.15kb +0.00b 6.95s -816.00ms 🚀
dist/ConfigPanelFieldsLoader.f06a6b36.js 312.08kb +0.00b 6.95s -815.00ms 🚀
dist/card.501ecffa.js 143.52kb +0.00b 6.95s -815.00ms 🚀
dist/ConfigPanelFieldsLoader.e1ae433f.js 83.45kb +0.00b 6.95s -815.00ms 🚀
dist/ElementBrowser.3bcad544.js 65.85kb +0.00b 6.95s -814.00ms 🚀
dist/archive.503fa405.js 61.48kb +0.00b 9.67s +1.91s ⚠️
dist/esm.945b66be.js 60.94kb +0.00b 6.95s -815.00ms 🚀
dist/component-lazy.60375b05.js 60.45kb +0.00b 5.14s +347.00ms ⚠️
dist/ConfigPanelFieldsLoader.ef739802.js 16.14kb +0.00b 6.95s -814.00ms 🚀
dist/ui.2de0ef21.js 14.88kb +0.00b 6.95s -815.00ms 🚀
dist/ConfigPanelFieldsLoader.c68d84ab.js 14.25kb +0.00b 6.95s -814.00ms 🚀
dist/pdfRenderer.187ba54d.js 12.21kb +0.00b 6.95s -815.00ms 🚀
dist/mobile-upload.136dd5cb.js 8.08kb +0.00b 6.95s -814.00ms 🚀
dist/mobile-upload.0bdb676c.js 8.08kb +0.00b 6.95s -815.00ms 🚀
dist/index.b16227d6.css 4.08kb -2.00b 🚀 9.79s -154.00ms
dist/media-viewer-analytics-error-boundary.e6109a6a.js 3.46kb +0.00b 9.67s +1.91s ⚠️
dist/ru.896915b9.js 2.94kb +0.00b 6.95s +1.66s ⚠️
dist/uk.48c97550.js 2.89kb +0.00b 6.95s -814.00ms 🚀
dist/codeViewerRenderer.915ef6b3.js 2.84kb +0.00b 9.67s +1.91s ⚠️
dist/th.31044730.js 2.73kb +0.00b 6.95s -814.00ms 🚀
dist/vi.d8dcb67a.js 2.22kb +0.00b 6.95s -814.00ms 🚀
dist/tr.46f26598.js 2.16kb +0.00b 6.95s -814.00ms 🚀
dist/sv.13d93533.js 2.10kb +0.00b 6.95s -814.00ms 🚀
dist/zh_TW.afaf6222.js 1.98kb +0.00b 6.95s -814.00ms 🚀
dist/zh.fcdc32bb.js 1.96kb +0.00b 6.95s -814.00ms 🚀
dist/workerHasher.ef49a7fc.js 1.72kb +0.00b 6.95s -814.00ms 🚀
dist/workerHasher.9d5fe27b.js 1.72kb +0.00b 6.95s -815.00ms 🚀
dist/heading5.023a8f1f.js 1.36kb +0.00b 5.14s +348.00ms ⚠️
dist/sk.101f1705.js 786.00b +0.00b 6.95s +1.66s ⚠️
dist/simpleHasher.f1f58b0a.js 687.00b +0.00b 6.95s -814.00ms 🚀
dist/simpleHasher.09f4d713.js 687.00b +0.00b 6.95s -815.00ms 🚀
dist/ro.a6eff34a.js 612.00b +0.00b 6.95s +1.66s ⚠️
dist/index.html 240.00b +0.00b 9.83s +5.36s ⚠️

Cached Bundles

Bundle Size Difference Time Difference
dist/media-viewer.bd165005.js 542.15kb +0.00b 7.01s -1.11s 🚀
dist/index.b16227d6.css 4.08kb -2.00b 🚀 9.78s +309.00ms
dist/heading4.05995ed9.js 1.25kb +0.00b 5.07s +440.00ms ⚠️

Three.js ✅

Timings

Description Time Difference
Cold 4.76s -16.00ms
Cached 265.00ms +1.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@alshdavid alshdavid requested a review from a team January 11, 2023 04:39
@mischnic
Copy link
Member

The tests are failing now, and there's a merge conflict

@mischnic
Copy link
Member

This currently excludes type imports like import type { Foo } from 'foo' due to the current version of eslint-plugin-import not including the change which adds support for type imports.

I am not sure when they will merge this, the last update was almost a year ago.

BTW, that was released in the meantime with 2.27

are declared in that package's package.json.

Note:
This will fail to lint due to correcting the import lint rule

Changelog:
- Updated eslint version to the latest 7.x.x release
- Updated the eslint plugins relevant to the import rule
- Updated the root eslintrc.json to include the import rule
  - Note that this rule must be in the root directory of the workspace
    See "packageDir" under "options" related to monorepos
    https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-extraneous-dependencies.md
- Corrected plugin resolution for @parcel/babel-preset
Changelog:
- Updated eslint errors unrelated to imports
- Updated @parcel/source-map version
- Added elm as a devDependency
- Ignoring node_modules/resolve from flow check
@mischnic mischnic force-pushed the alsh/fixing-eslint-dependency-rule branch from 581d539 to 15bec7d Compare March 26, 2023 10:31
@mischnic mischnic force-pushed the alsh/fixing-eslint-dependency-rule branch from 4c42950 to fbc919f Compare March 26, 2023 11:26
@alshdavid alshdavid closed this May 16, 2023
@mischnic mischnic deleted the alsh/fixing-eslint-dependency-rule branch September 26, 2023 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants