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

Backport typescript fixes from #2555 to v6.5.0 #2600

Open
wants to merge 8 commits into
base: support/6.x
Choose a base branch
from

Conversation

RobinVdBroeck
Copy link
Contributor

@RobinVdBroeck RobinVdBroeck commented May 2, 2024

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.

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 script sync-tsconfigs that writes the tsconfig.json to every package.

tsconfig.shared.json Outdated Show resolved Hide resolved
@RobinVdBroeck RobinVdBroeck marked this pull request as draft May 2, 2024 22:23
.monorepolint.config.ts Outdated Show resolved Hide resolved
@RobinVdBroeck RobinVdBroeck changed the title Draft: Backport typescript fixes from #2555 to v6.5.0 Backport typescript fixes from #2555 to v6.5.0 May 4, 2024
@RobinVdBroeck RobinVdBroeck marked this pull request as ready for review May 4, 2024 22:34
tsconfig.shared.json Outdated Show resolved Hide resolved
@RobinVdBroeck
Copy link
Contributor Author

RobinVdBroeck commented May 13, 2024

Hey, this PR has been open for a while. Is there any way I can help accelerate merging this?

@smallsaucepan
Copy link
Member

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?

@RobinVdBroeck
Copy link
Contributor Author

arethetypeswrong report no problems, both for the bundle in @turf/turf and the subpackages.

image image

@elvince
Copy link

elvince commented Jun 6, 2024

Hi,

Can we expect a new release with thoses changes soon?
it will be really valuable to get typings working correctly in v6.5.0

Thanks,

@smallsaucepan
Copy link
Member

smallsaucepan commented Jul 27, 2024

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.

@wmertens, @VIKTORVAV99, @elvince

@RobinVdBroeck
Copy link
Contributor Author

@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:

  1. Use pnpm instead of yarn so it matches v7 (this is not currently in this PR).
  2. Update lerna so it matches v7 (this is not currently in this PR).
  3. Update TypeScript so we can use the correct bundle resolution.
  4. Introduce tsx + TypeScript fix.

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.

@smallsaucepan
Copy link
Member

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?

@smallsaucepan
Copy link
Member

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.
@RobinVdBroeck
Copy link
Contributor Author

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.
@smallsaucepan
Copy link
Member

Will review the PR with the combined changes and see if it makes it through a build 🤞

@mfedderly
Copy link
Collaborator

I have a couple things to think about for this PR:

  1. Upgrading to 7.0.0 is hopefully not that big of a change, I migrated two large monorepos with lots of separate Turf consumers. The changes mostly fell into a few categories: using @types/geojson instead of @turf/helpers, using the named exports instead of the defaults, and some small breaking changes to the APIs of @turf/union and @turf/difference. Hopefully you aren't stuck on 6.x for too much longer.
  2. I didn't look at this PR too closely, but I'd be worried about introducing breaking changes to packages unintentionally. So just be careful around things like the default exports.

@smallsaucepan
Copy link
Member

smallsaucepan commented Aug 7, 2024

Thanks @mfedderly. Both good points.

  1. We don't necessarily have to do this. I guess I'm guided by what little feedback we have i.e. someone raised an issue and it got three thumbs up.
  2. Definitely. Goal at the moment will be to see if we can get it to build in CI. Then will do a side by side comparison of the 6.5.0 generated code and this.

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.

4 participants