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

Big PR [DO NOT MERGE] #253

Closed
wants to merge 8 commits into from
Closed

Conversation

lishaduck
Copy link
Contributor

@lishaduck lishaduck commented Sep 19, 2024

XC has taken a big drain out of my time and energy, and I'm primarily posting this for posterity.
Someday, I'll rebase this and make it reviewable, for now, I just want to note some improvements that could be made by someone, hopefully me?

P.S., I know no stress 🙃 That's why I'm posting this, to put it somewhere I can know it's backed up and be accessed as needed.


Children:

Ok, so this one is complicated.
SemVer wise, we should be able to support glob 10 & glob 11.
The only breaking change is the engine.
Therefore, the "correct" specifier is indeed `^10.2.6 || ^11.0.0`.
We support both, if you need an older engine,
you should use an older version.
In fact, npm is supposed to do this automatically.
However, up until `npm-pick-manifest@9.1.0` (included in `npm@10.8.2`),
there was a longstanding bug where it didn't!
The code was there,
but the logic didn't work right and after Isaacs moved on to vlt,
his PR fixing this got stalled.
After the `glob` fiasco, he rebased the PR and it's now fixed,
but the change wasn't backported.
Because we support Node 14, we support `npm@6.14.18`,
so we can't take advantage of this.
Instead, we just have to tolerate a slightly old version of glob.
Oh well.

I also removed "enginesStrict" because our engine declaration is now higher than technically needed
because glob only declares support for the latest versions of Node branches.
- Sort `require`s.
- Move type imports above real imports.
- Switch to `@import` for imports needing `resolution-mode`.
- Enable `noUnusedLocals`.
- Move exports to the bottom of files.
- Switch an inline type import to an import declaration.
- Use `... = require('...').default` over `{default: ...} = require('...')`.
- Refactored `spinner.js` exports to be typesafe.
- Bumped TypeScript to fix some bugs.

fix: type error

Less dependency injection 🙁
Copy link

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/ls-engines@0.9.3 Transitive: environment, eval, filesystem, network, shell, unsafe +264 14.8 MB ljharb
npm/terminal-link@2.1.1 None +3 149 kB sindresorhus
npm/turbo@2.0.5 None +6 128 MB turbobot
npm/typescript@5.5.4 None 0 21.9 MB typescript-bot
npm/uglify-js@3.18.0 environment, eval, filesystem 0 1.3 MB alexlamsl
npm/which@2.0.2 environment Transitive: filesystem +1 20.9 kB isaacs

🚮 Removed packages: npm/ls-engines@0.9.2

View full report↗︎

@lishaduck
Copy link
Contributor Author

Oh, I didn't even push the big branch. This is the small branch... 🤣

This was referenced Oct 31, 2024
@lishaduck
Copy link
Contributor Author

Closing as superseded.

@lishaduck lishaduck closed this Nov 3, 2024
@lishaduck lishaduck deleted the fix-imports branch November 3, 2024 22:44
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.

1 participant