-
Notifications
You must be signed in to change notification settings - Fork 944
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
Backport typescript fixes from #2555 to v6.5.0 #2600
base: support/6.x
Are you sure you want to change the base?
Conversation
44a9ee6
to
de5bb32
Compare
Hey, this PR has been open for a while. Is there any way I can help accelerate merging this? |
Hi @RobinVdBroeck. Thanks for submitting the PR. Happy to work with you to get it merged. I do agree with your there is value in backporting some fixes to the v6 branch as well. Please be aware though that all our time and energy at the moment is dedicated to getting v7 out. To cover off an additional testing step, have you tried using arethetypeswrong to check the generated JS and typedefs? |
Hi, Can we expect a new release with thoses changes soon? Thanks, |
5915161
to
d110abc
Compare
Hi @RobinVdBroeck. Revisiting this PR and noted for it to build with our current CI setup it would need to be pnpm based. Started looking at that and it's getting a bit "whack-a-mole"-ish. Every problem I solve leads to two more appearing. Will persist a bit longer. In the meantime, what would be your thoughts if instead we ported the latest build system and packaging infrastructure (what we released v7 with) over to the support/6.x branch, leaving the actual Turf code as is? In other words a 6.x release cut using contemporary v7 infrastructure and improved packaging, but with 6.5.0 functionality as the Javascript level. |
@smallsaucepan Could you please clarify what you need from me? I think you're asking me to open the following PRs, but I'm not entirely sure:
From a JavaScript point of view, nothing has changed in this PR. I haven't changed any source code. I believe there's value in handling steps 1-3 in a separate PR to make this one easier to understand. |
Sorry I was vague @RobinVdBroeck. Not asking you to do anything or open new PRs at the moment. More asking for your thoughts on a possible alternate way to a typescript friendly 6.x release. I was hoping to add on to your PR an upgrade to pnpm as well so we could build and publish the 6.x bundle with our current build script. This proved problematic, which lead me to wonder if we could approach from a totally different direction. Instead of upgrading parts of the v6 plumbing as you've done in this PR, could we instead effectively take a copy of the v7 branch, and copy in the v6 business logic only, packaging that up instead. I'm experimenting with that now, though would be interested in what you think of that approach? |
Ok, have managed to add to this PR so that it will build with our current github CI (minimal pnpm and lerna changes). @RobinVdBroeck do you mind if I push those changes here? |
… for building and releasing the 6.x branch using our current CI setup. Removing empty turf-geojson-rbush directory and getting monorepolint upgraded and the 6.5.0 checks ported.
Yeah no problem, feel free to do that. I guess it comes down to the same thing. |
…ck to moduleResolution node and target es5 to keep differences from 6.5.0 to a minimum. Upgraded glob usage and removed references to dist/ generated files being required where possible.
Will review the PR with the combined changes and see if it makes it through a build 🤞 |
I have a couple things to think about for this PR:
|
Thanks @mfedderly. Both good points.
|
Please fill in this template.
npm test
at the sub modules where changes have occurred.npm run lint
to ensure code style at the turf module level.Doing an upgrade to 7.0.0 might be a big endeavor for some users of this library, so this would allow them to use modern Typescript features (like node16 module resolution) or use a bundler like vite.
I've backported the changes done in #2555 to v6.5.0. I haven't backported the pnpm changes, since that seems to be out of scope for this backport.
I've bumped the version of typescript here to 4.7.2, since that is the first one to introduce the
node16
module resolution algorithm.I've manually verified the changes by setting up a minimal vite project, which was previously incompatible with v6.5.0, and using my local build (by yarn link). I've also setup a small script (
const foo = require("@turf/union").default; console.log(foo != undefined
) to make sure that commonjs is not broken.tsup requires that there is a
tsconfig.json
file in every package, so I've added a scriptsync-tsconfigs
that writes the tsconfig.json to every package.