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

chore: bump and align dependencies; dedupe typescript #379

Merged
merged 9 commits into from
Oct 24, 2023

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented Jun 8, 2023

This bumps and aligns dependency versions to resolve inefficiencies and fixed upstream issues.

  • [email protected] currently pulls in multiple different versions of typescript. This removes the 3.x and 4.x transitive dependencies, aligning on a single ^5.0.4, which is now also the version used as devDependency.
  • Remove redundant explicit detective-* packages which are already included through precinct
  • Bump dependencies
    • Some dependency packages have been updated to ESM modules. Updating to latest for these packages will require converting madge to ESM as well, which should be done as a future breaking change in madge v7 or later. These have been kept at the latest CommonJS version.
  • test: Remove specific typescript configuration for ASP.net

This reduces the size of node_modules on a local checkout from 333MB to 149MB.

@legobeat legobeat marked this pull request as ready for review June 8, 2023 06:25
@legobeat legobeat force-pushed the deps-dedupe-typescript branch from 8f91d2b to afc3a42 Compare June 8, 2023 06:29
@vbraun
Copy link

vbraun commented Jun 14, 2023

This also updates dependencies

"dependency-tree": "^10.0.9"
   --> "filing-cabinet": "^4.1.6",

which would pull in dependents/node-filing-cabinet#117 which should fix some breakage with index.ts files

@legobeat
Copy link
Contributor Author

@pahen @kamiazya

@legobeat
Copy link
Contributor Author

legobeat commented Sep 3, 2023

@PabloLION @pahen @kamiazya PTAL

@PabloLION
Copy link
Collaborator

Hi, I'm just back to dev.
Thanks for this great PR. Aligning ts version was what I wanted to do. Great to see someone actually did it. And those detective-* are never explicitly imported in the code so removing them are reasonable.
As for the ESM conversion, I remembered that @pahen was against it, right? If not we can do that in v7.

test/typescript.js Show resolved Hide resolved
@PabloLION PabloLION self-requested a review October 22, 2023 22:55
@PabloLION
Copy link
Collaborator

Hi, @legobeat ! thanks for the PR and I think it's almost ready to get merged! Only thing is that I find the base of your branch is 499 commits ago. Could you rebase it to current master? I tried to do it but it seems you didn't select "Allow edits from maintainers" so my hands are tied.
Thanks for the PR again!

@legobeat legobeat force-pushed the deps-dedupe-typescript branch from afc3a42 to b5aeefe Compare October 22, 2023 23:24
@legobeat
Copy link
Contributor Author

@PabloLION Hm, odd, the "Allow edits and access to secrets by maintainers" box is and was checked.

Either way, just rebased on current master.

@PabloLION
Copy link
Collaborator

Sorry I just logged off. This will be the first priority on my next session. Thanks again!

@legobeat
Copy link
Contributor Author

legobeat commented Oct 22, 2023

@typescript-eslint/typescript-estree 6.8 does not support TS>5.2.0
@PabloLION
Copy link
Collaborator

PabloLION commented Oct 23, 2023

I found TS>5.2.0 gets a warning (no error) with @typescript-eslint/typescript-estree So I'm limiting it to 5.2.0. I think it's enough for our current stage. And since it's already in the dev dependency, we no longer need it as a peer dep nor in the peer dep meta. So I removed those fields.
How does this look to you @legobeat ? If you are ok with these changes, I'll proceed with the merging now.

devDeps: pin to typescript ~5.2.0 in devDeps only

@typescript-eslint/typescript-estree is only used for linting and
shouldn't affect runtime dependencies.
@legobeat
Copy link
Contributor Author

legobeat commented Oct 23, 2023

I found TS>5.2.0 gets a warning (no error) with @typescript-eslint/typescript-estree So I'm limiting it to 5.2.0. I think it's enough for our current stage. And since it's already in the dev dependency, we no longer need it as a peer dep nor in the peer dep meta. So I removed those fields. How does this look to you @legobeat ? If you are ok with these changes, I'll proceed with the merging now.

Hm, so my observations:

  • @typescript-eslint/typescript-estree is only used as a devDependency for linting.
    • While it seems reasonable to restrict the version used as devDependency, this shouldn't affect what versions can be used for runtime.
  • devDependencies is not recognized for users / dependents of the package. They are only relevant for devs, ie when running ${PACKAGEMANAGER} install in the repo.
  • Did anything other related to this change since Move typescript to peer dependencies #350 / constrain and bump typescript versions #376? Otherwise seems like the motivations for keeping it as optional peerDependency would still be as valid?

Did I miss something here?

Just pushed 726d2c2, which restricts the version of the devDependency but retains the peerDependency

Separately, checking in the lockfile (part of #394) would improve reproducability and consistency with regards to devDependencies after it is merged.

@PabloLION
Copy link
Collaborator

PabloLION commented Oct 24, 2023

Sorry that I was sloppy on 9db3ebd

Your statements are all correct, I'm just trying to relate them to 726d2c2

  • @typescript-eslint/typescript-estree is only used as a devDependency for linting.
    • While it seems reasonable to restrict the version used as devDependency, this shouldn't affect what versions can be used for runtime.

We should restrict typescript version as devDependency

Should keep TS as peerDependency for non-dev users.

Finally, thanks again for the correction and the work.

@PabloLION PabloLION merged commit 765f25c into pahen:master Oct 24, 2023
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