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

Migration to pnpm and improved exports strategy #2555

Merged
merged 17 commits into from
Dec 18, 2023

Conversation

smallsaucepan
Copy link
Member

Another tranche of changes designed to modernise the build:

  • Migrating repo from old yarn to modern pnpm
  • Improved our exports strategy to better support both CJS and ESM
  • Using named exports / imports within Turf (keeping default exports for now)
  • Dropping support for ie11

NOTE couldn't get monorepolint working with pnpm, so it is currently disabled. MRL project shows signs of recent activity so will revisit with a more recent release in the near future.

Fixes #2553 and fixes #2260.
Also fixes #2307 though that was really remained open for discussion purposes.

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Have read How To Contribute.
  • Run npm test at the sub modules where changes have occurred.
  • Run npm run lint to ensure code style at the turf module level.

…y up our cjs / esm situation using tsup (instead of tsc and rollup). Can't get monorepolint to work with new setup so disabling for the time being.
…named exports. Updating github workflow actions to pnpm instead of yarn.
…". Not tracking nodenext just yet. Changing tsup command to generate d.ts files that arethetypeswrong approves of.
…ts, and ordering alphabetically. Added in a couple of packages - convex, booleanValid and nearestNeighbourAnalysis. Rollup now only used in packages/turf so merging project root base config into there. Also now only using rollup in packages/turf to do final conversion to web module. JS and d.ts files generated the same as other modules, using tsup.
…ripping back tsconfig.json to the bare minimum. Adding tsconfig.json to packages that while still only JS, are now processed by tsup.
…e subset of packages that had multiple entry points e.g. 3rd party code hosted locally in lib/
…h d.ts entries. Turfjs#2307 (comment)  Specifying overall module type as commonjs explicitly.
…had on build target command line earlier in this PR in to tsup config file. Also disabling treeshaking as this was generating a warning for CJS files and causing the workaround above to not work.
…browserslist config in babel.config.json e.g. previous chrome 67 and edge 17 are 5 years old, ie11 is just ... ugh
Copy link
Contributor

@favna favna left a comment

Choose a reason for hiding this comment

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

Migrating repo from old yarn to modern pnpm

Any particular reason to not go from old yarn to modern yarn? As in, Yarn v4. To use one of the copypasta tags from my Discord server:

Click to expand, includes all the info and copypastable commands for switching to it

Yarn v4 is a new version of Yarn that we recommend switching to as Yarn v1 has long since been deprecated.

"But I don't see any update on [source]?"

That is correct. Yarn v4 is installed through Yarn itself. You configure Yarn v4 on a per-project basis. How you installed Yarn globally is largely irrelevant to this (corepack, volta, something else). How to install Yarn v4 for your project? Simply write:

yarn set version berry

This will download the new Yarn v4 binary and put it in .yarn/releases, you should push this to your Git repository. It will also create a .yarnrc.yml file which configures the path which you should also commit.

Next you probably also want to run the following 2 commands:

yarn config set enableGlobalCache true
yarn config set nodeLinker node-modules

This will add to your .yarnrc file:

enableGlobalCache: true
nodeLinker: node-modules

This ensures you have a more traditional experience with node_modules and a global cache.

The next step is to nuke your node_modules and yarn.lock and run yarn install

Then some final adjustments. Put this in you .gitignore:

# Yarn files
.yarn/install-state.gz
.yarn/build-state.yml

And anywhere in your scripts in package.json where you use * you should wrap it in extra "
For example:

{
	"format": "prettier --write \"src/**/*.ts\""
}

Mind you this last thing is a good thing to add regardless of script runner/package bundler because it ensures the glob is performed by the library and not by your shell, which may differ when people develop on different operating systems.

In short, the command to set everything up you can run:

yarn set version berry && yarn config set enableGlobalCache true && yarn config set nodeLinker node-modules && echo "" >> .gitignore && echo "# Yarn files" >> .gitignore && echo ".yarn/install-state.gz" >> .gitignore && echo ".yarn/build-state.yml" >> .gitignore

Ultimately Yarn v4 is

  • Faster than Yarn v1, pretty much on par with pnpm
  • Is very configurable
  • Documentation is phenomenal
  • I don't know how it works with pnpm as I don't use it but keep in mind that with Yarn you need to run yarn npm publish as opposed to npm publish so Yarn resolves workspace:^ dependencies before publishing since that's obviously not valid for the npm registry. I assume for pnpm you'd need pnpm publish as well.
  • Still sticks to node_modules. pnpm with their pnp module has some weird things from time to time in specific project stacks, such as when module augmenting with TypeScript of transitive dependencies and pnpm doesn't link the transitive dependencies to node_modules.. yeah it's a bit of a PITA sometimes. Probably doesn't affect Turf but personally it has really annoyed me about pnpm.

FWIW I'm not really sure what your status is in relation to the Turf org but I imagine the chances for the PR to get merged are far far higher by sticking to Yarn.

.github/workflows/turf.yml Outdated Show resolved Hide resolved
@smallsaucepan
Copy link
Member Author

Thanks @favna. Ha ha I did have a yarn 4 PR almost ready though couldn't get it to work with GitHub CI (something corepack related). About the same time another of the Turf team mentioned we weren't necessarily married to yarn. So to get a release out asap just went with pnpm instead.

Perhaps we will revisit yarn down the track. Things like constraints would be nice, though might also take a look at syncpack.

Noted your comments about releasing too. Will keep an eye on that as we go.

Co-authored-by: Jeroen Claassens <jeroen.claassens@live.nl>
@favna
Copy link
Contributor

favna commented Dec 18, 2023

GitHub CI (something corepack related)

I'm curious to know what. This is not something I ever experienced. I always just use setup-node action and then just run yarn install --immutable (equivalent of yarn install --frozen-lockfile). Are you perhaps misunderstanding how Yarn v4 is installed and configured?

TL;DR of the collapsed content above, you install Yarn v4 on a per-project basis and the binary gets put in the .yarn/releases directory which you're supposed to check into Git. Then the global Yarn, whichever version that may be is irrelevant, knows to look for the .yarnrc.yml file and the yarnPath config in there and use the binary as specified by the path.

This has the great advantage of ensuring that every developer on the project uses the same toolchain and same version, thus ensuring that there are no discrepancies on that part when trying to help your co-workers*. As an anecdote of sorts, in the past I've had colleagues working on Linux while most of us worked on MacOS and whenever the Linux colleagues ran npm install the package-lock.json would change in terms of URLs, versions resolved and whether http or https was used even though we were using the same versions of Node and Npm. Thanks OS differences... never again for me, please.

* I also do Java development and for this reason, I also much prefer Gradle over Maven because it has the exact same advantage

Aanyywaayy, ultimately do as you please of course. It's your project at all and it sounds like you guys talked about it well enough so I won't butt in further.

@smallsaucepan
Copy link
Member Author

Are you perhaps misunderstanding how Yarn v4 is installed and configured?

Oh, that is entirely possible 😅 The v4 PR was #2551 if that's of interest, and the CI corepack problem is being tracked in an open issue in actions/setup-node (actions/setup-node#531)

Aanyywaayy, ultimately do as you please of course. It's your project at all and it sounds like you guys talked about it well enough so I won't butt in further.

No, this is all good intel and welcome any time. Even if we don't use something right away it's good to know what sort of things to look out for.

Copy link
Collaborator

@mfedderly mfedderly left a comment

Choose a reason for hiding this comment

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

This is a huge amount of work. Thank you!

"lint:escheck-cjs": "es-check es8 packages/*/dist/cjs/index.cjs packages/turf/turf.min.js",
"lint:escheck-esm": "es-check --module es8 packages/*/dist/esm/index.mjs",
"lint:escheck-web": "es-check es5 packages/turf/turf.min.js",
"lint:docs": "documentation lint packages/turf-*/index.js packages/turf-*/index.mjs",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work with .ts modules?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean "documentation lint"? Good question. I'll check and we can include turf-*/index.ts as a seperate change.

@@ -194,3 +194,6 @@ function B(t: number) {
(1 - t) * (1 - t) * (1 - t),
];
}

export { Spline, Point };
export default Spline;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might consider marking these all as @deprecated (in another PR) if we're intending to remove them eventually

@smallsaucepan smallsaucepan merged commit 4ea81e6 into Turfjs:master Dec 18, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants