Skip to content

Fix lodash-es imports#7509

Merged
zoran995 merged 2 commits intoTerriaJS:mainfrom
pjonsson:fix-lodash-imports
Mar 5, 2025
Merged

Fix lodash-es imports#7509
zoran995 merged 2 commits intoTerriaJS:mainfrom
pjonsson:fix-lodash-imports

Conversation

@pjonsson
Copy link
Copy Markdown
Contributor

What this PR does

This package depends on lodash-es,
so import everything from that
dependency.

Test me

I believe this is tested by CI?

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

@zoran995
Copy link
Copy Markdown
Collaborator

It might be good to enforce this as an eslint rule https://eslint.org/docs/latest/rules/no-restricted-imports. @ljowen, what do you think?

@pjonsson
Copy link
Copy Markdown
Contributor Author

pjonsson commented Feb 28, 2025

Before, I have been finding this stuff manually, but this week I configured Knip to find missing/unused dependencies and files for me. There's no point in adding a tool that constantly says "FAIL FAIL FAIL" though, so I was planning to make a PR with a Knip configuration after the worst offenses had been fixed.

This PR, and #7506 were both found because of Knip.

Edit: Knip complains about a lot of stuff I've found manually already too, the unused dependency PRs that Zoran has merged the last couple of days, along with #7397, #7399, #7411, #7484, #7491, #7492, and #7495.

@pjonsson pjonsson force-pushed the fix-lodash-imports branch from ed846e0 to 814e515 Compare March 4, 2025 09:47
@zoran995
Copy link
Copy Markdown
Collaborator

zoran995 commented Mar 4, 2025

I am aware of Knip and have used it on various projects in the past; however, it does not provide feedback in real-time like ESLint does. You need to run it manually, which typically means deferring feedback for a developer to CI. That’s why I suggest setting up an ESLint rule to manage this and have it warn/error during development.

@ljowen
Copy link
Copy Markdown
Contributor

ljowen commented Mar 5, 2025

https://eslint.org/docs/latest/rules/no-restricted-imports

I'm in favour of the lint rule :)

pjonsson added 2 commits March 5, 2025 11:02
This package depends on lodash-es,
so import everything from that
dependency.
This package uses lodash-es,
so import that instead.
@pjonsson pjonsson force-pushed the fix-lodash-imports branch from 814e515 to de7a0ed Compare March 5, 2025 10:09
@pjonsson
Copy link
Copy Markdown
Contributor Author

pjonsson commented Mar 5, 2025

Added the requested ESLint configuration.

@ljowen ESLint 9 has deprecated/removed a bunch of checks and uses a flat configuration format which is quite different from the current format. The longer #7437 sits in the PR queue, the greater the risk is that some configuration changes will be lost in the upgrade when you finally decide to stop using the deprecated ESLint 8.

@zoran995 zoran995 merged commit f080cb1 into TerriaJS:main Mar 5, 2025
@pjonsson pjonsson deleted the fix-lodash-imports branch March 5, 2025 13:51
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