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(monorepo): separate each loader into its own package #23

Merged
merged 38 commits into from
Nov 14, 2024

Conversation

JakobJingleheimer
Copy link
Owner

closes #19

AugustinMauroy
AugustinMauroy previously approved these changes Sep 19, 2024
@AugustinMauroy
Copy link
Collaborator

AugustinMauroy commented Nov 8, 2024

Feedback

  • Use 22 in .nvmrc to use LTS
  • Update dep to just have @types/nodejs because we don't need specific packages.

Is .npmignore still need ?

@JakobJingleheimer
Copy link
Owner Author

  • Use 22 in .nvmrc to use LTSpackages.

I added backport requests to module.findPackageJSON(). I'm not sure if those will happen immediately. It will be available in 23.2 though.

  • Update dep to just have @types/nodejs because we don't need specific

I don't understand—isn't that what I did? (The package is @types/node btw)

Is .npmignore still need ?

Yes, very much: That's what prevents dev files getting published.

@JakobJingleheimer
Copy link
Owner Author

@AugustinMauroy I added some commits on top of a WIP commit. I'll finish my WIP commit, then cherry-pick your stuff back on top, and then force-push.

@AugustinMauroy
Copy link
Collaborator

I don't understand—isn't that what I did? (The package is @types/node btw)

yup my bad I didn't see you remove it from sub packages

@AugustinMauroy
Copy link
Collaborator

I added backport requests to nodejs/node#55412. I'm not sure if those will happen immediately. It will be available in 23.2 though.

is it mean loader will only work for 23.2 ??? (for example nodejs.org still on 20)

@JakobJingleheimer
Copy link
Owner Author

JakobJingleheimer commented Nov 8, 2024

is it mean loader will only work for 23.2 ??? (for example nodejs.org still on 20)

For tsx loader, unfortunately yes (until node:module.findPackageJSON() gets backported, which may be right away). I can't think of another way to find esbuild.config. Would love another idea! (we can't use process.cwd()).

As-is, we can't release this change until findPackageJSON is released.

EDIT: I spoke with Antoine, and 23.2 is being prepared now, then it will be backported to 22.x and 20.x 2 weeks later. So let's target Mon, 25 November for the nodejs-loaders v2 release.

Copy link
Collaborator

Choose a reason for hiding this comment

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

now we can put in gitignore because we publish it with GA

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm not sure: I committed it because a comment is supposed to get posted with a coverage diff. I think in order to do that, coverage needs to get committed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Honslty I'm not sure. It's first time I work on a package. But with jest we didn't commit the coverage.

Copy link
Collaborator

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

LGTM ! Just one remaining thing should we merge before back port or wait backport.
Or merge but wait for publishing.
Any strong opinions

@JakobJingleheimer JakobJingleheimer merged commit 69d6638 into main Nov 14, 2024
2 of 4 checks passed
@JakobJingleheimer JakobJingleheimer deleted the chore/monorepo branch November 14, 2024 20:07
@AugustinMauroy
Copy link
Collaborator

WOW happy to see this happened !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish loaders as separate npm packages
2 participants