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

Upgrade to yarn v4 #2551

Closed
wants to merge 11 commits into from
Closed

Conversation

smallsaucepan
Copy link
Member

Turf was using a very old version of yarn (v1), with v4 being the most recently released. Upgrading to a newer version (apart from v1 eventually not being supported) we get access to some monorepo sugar - "workspace:^" instead of "v7.0.0-alpha.2" etc, and first class yarn workspaces commands. There might be some npm deployment related changes, though this has been around long enough they should be well documented. Tagging @mfedderly for a review regardless.

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.

… --immutable. Updating module dependencies to use special "workspace:" syntax. Should remove need to update all package.json Turf dependencies for every Turf release.
…sn't run user defined pre* scripts automatically any more, so pretest, posttest, and postlint have been rolled into their respective "main" tasks. Adding yarn prepare explicitly to github workflow as yarn doesn't seem to call that implicitly as part of install.
…re though think I checked in a copy before that was in place.
@smallsaucepan smallsaucepan removed the request for review from mfedderly December 7, 2023 08:36
@smallsaucepan
Copy link
Member Author

Drat. Looks like the node-setup github action can't enable corepack, which we need to install the newer yarn version in the CI environment. Going to move this PR in to draft status until that is possible or I can find another way to press on 😩

Dependent on PR actions/setup-node#901

@smallsaucepan smallsaucepan marked this pull request as draft December 7, 2023 08:56
@mfedderly
Copy link
Collaborator

For what its worth, I don't think we are tied to yarn for any particular reason (I moved us there because we didn't even have a node package lock at the time). I've mostly moved over to pnpm if that helps make this PR easier.

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.

Overall lgtm, happy to re-review once it builds.


isInside = booleanPointInPolygon(multiPoint.coordinates[i], polygon, {
ignoreBoundary: true,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is a no-op (I guess?) but its confusing as part of the yarn upgrade. It also might be a step away from a better version where we can skip the second booleanPointInPolygon? Not really sure what the original intent was of the oneInside boolean.

Copy link
Member Author

Choose a reason for hiding this comment

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

Me either. Yeah, it basically evaluated to if (true) ...

Was an issue that didn't seem to justify its own PR, though happy to separate it out.

@smallsaucepan
Copy link
Member Author

For what its worth, I don't think we are tied to yarn for any particular reason (I moved us there because we didn't even have a node package lock at the time). I've mostly moved over to pnpm if that helps make this PR easier.

Well, I might see how this goes with pnpm instead and if that's smoother scrap this PR for one based around that tool. Thanks for clarifying.

@smallsaucepan
Copy link
Member Author

Retiring in favour of PR #2555

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.

2 participants