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

Migrating remaining JS modules to Typescript (part 1) #2543

Merged
merged 22 commits into from
Nov 28, 2023

Conversation

smallsaucepan
Copy link
Member

Migrating remaining Javascript modules to Typescript (the main implementation file at least). More discussion in #2538

This PR covers:

turf-difference
turf-dissolve
turf-ellipse
turf-envelope
turf-explode
turf-flatten
turf-flip
turf-point-on-feature
turf-points-within-polygon
turf-polygon-smooth
turf-polygon-tangents
turf-rewind
turf-sample
turf-sector
turf-shortest-path
turf-simplify
turf-tag

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.

packages/turf-dissolve/index.ts Show resolved Hide resolved
packages/turf-dissolve/index.ts Outdated Show resolved Hide resolved
packages/turf-dissolve/index.ts Outdated Show resolved Hide resolved
packages/turf-flatten/index.ts Show resolved Hide resolved
packages/turf-flip/index.ts Outdated Show resolved Hide resolved
packages/turf-sample/index.ts Show resolved Hide resolved
packages/turf-shortest-path/index.ts Show resolved Hide resolved
packages/turf-shortest-path/index.ts Show resolved Hide resolved
packages/turf-simplify/lib/simplify.d.ts Show resolved Hide resolved
… turf-flip package.json in an earlier commit.
…n't working just yet (mixed Points and MultiPoints). Need to confirm this is actually valid use of the function.
…t a mixture of Point and MultiPoint features.
…d here. First was that an unnecessary parameter (polygon) was being passed to processPolygon. Second was enext param passed into processPolygon was simply being overwritten and not returned - converted to a local const. Noting in history in case a future developer is chasing a bug.
…to turf-sector (test existed, just weren't being run).
…nor monorepolint fixes. Overloading a few function signatures so types.ts test pass. Fixing a few places where option parsing / defaults was half baked. Fixing a couple of modules with built in libs that were building locally but not when doing the final turf CDN build.
…fjs#2538 (comment) Changing options handling to not change the options object in place. Removing a couple of missed d.ts files.
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.

looks like the force push hid all the changes from the github review window for me, but I'm going to just approve to unblock since we're definitely working different hours at the moment. Please clean up the license stuff before merging though.

I think we've been using /*! to denote license comments
https://github.com/Turfjs/turf/blob/master/packages/turf-great-circle/lib/arc.js#L1

@smallsaucepan
Copy link
Member Author

Whoops. Sorry Matt. To summarise, agreed with pretty much everything you pointed out and will address with another commit shortly.

Only things I'm not sure I could address are some overloads and local lib typings. For overloads will leave as is as they seem more readable to the casual user. For local lib typings will put license in as a temporary measure and if there's something published (or that we can update easily) on Definitely Typed will tidy up in a subsequent PR.

Thanks for approving!

… handling in a few places, fixing overloaded function typedefs, reinstating some incorrectly removed runtime checks, adding license details for types we pulled in for locally hosted libraries.
@smallsaucepan smallsaucepan merged commit 66f9b2d into Turfjs:master Nov 28, 2023
3 checks passed
smallsaucepan added a commit to smallsaucepan/turf that referenced this pull request Dec 1, 2023
…to do - waiting for PR Turfjs#2543 to merge first to avoid conflicts.
smallsaucepan added a commit that referenced this pull request Dec 5, 2023
… exports to modules (#2550)

* Converting bench.js and test.js to Typescript. Adding along as a named export as well as the default.

* Converting bench.js and test.js to Typescript and ESM import. Adding angle as a named export as well as the default.

* Converting bench.js and test.js to Typescript and ESM import. Adding area as a named export as well as the default.

* Making turf last-checks a bit more flexible as we will have a mix of JS and TS test files for a while.

* Converting bench.js and test.js to Typescript and ESM import, and adding named exports as well as the default export for modules:

bbox
bboxCliip
bboxPolygon
bearing
bezierSpline
booleanClockwise
booleanConcave
booleanContains
booleanCrosses
booleanDisjoint
booleanEqual
booleanIntersect
booleanOverlap
booleanParallel
booleanPointInPolygon
booleanPointOnLine
booleanTouches
booleanValid
booleanWithin

* Converting bench.js and test.js to Typescript and ESM import for remaining Typescript modules. Also adding named exports in addition to the default export for those same modules.

* Proceeding with migrating remaining tests to Typescript. Still a few to do - waiting for PR #2543 to merge first to avoid conflicts.

* Converting bench.js and test.js to Typescript and ESM import for some more Typescript modules (postponed until recent PRs merged). Also adding named exports in addition to the default export for those same modules.

* Banishing a couple more require() calls from module code.

* Retiring custom rollup typescript plugin. Problem it was designed to address probably fixed by a previous typescript upgrade. Removing redundant es5 checking script that hasn't been used for a while as well.
@markcarroll
Copy link

markcarroll commented Dec 12, 2023

@smallsaucepan @mfedderly can we get a version bump of the 7.0 alpha so we can test these out?

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.

3 participants