-
Notifications
You must be signed in to change notification settings - Fork 606
TypeScript #1373
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
base: main
Are you sure you want to change the base?
TypeScript #1373
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prettier changed the formatting here.
| "test": "node --test --import ./test/mock-browser.js", | ||
| "coverage": "node --test --import ./test/mock-browser.js --experimental-test-coverage", | ||
| "test": "tsx test/*.test.ts", | ||
| "coverage": "tsx test/*.test.ts --experimental-test-coverage", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For --experimental-test-coverage to run, Node v22 must be used.
src/events.ts
Outdated
| zoomend: function() { | ||
| ctx.store.changeZoom(); | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looked like a dead/old method as store so I removed it.
| "docs": "run-s docs-modes-life-cycle docs-modes-get-and-set", | ||
| "docs-modes-get-and-set": "documentation readme --readme-file ./docs/MODES.md -s \"Setters and Getters\" src/modes/mode_interface_accessors.js --shallow", | ||
| "docs-modes-life-cycle": "documentation readme --readme-file ./docs/MODES.md -s \"Life Cycle Functions\" src/modes/mode_interface.js --shallow", | ||
| "lint": "eslint index.js src test bench", | ||
| "docs-modes-get-and-set": "documentation readme --readme-file ./docs/MODES.md -s \"Setters and Getters\" src/modes/mode_interface_accessors.ts --shallow", | ||
| "docs-modes-life-cycle": "documentation readme --readme-file ./docs/MODES.md -s \"Life Cycle Functions\" src/modes/mode_interface.ts --shallow", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what to do here. JSdoc have largely been replaced with TypeScript 🤔
|
|
||
| assert.deepEqual( | ||
| Draw.getAll().features[0].geometry.coordinates, | ||
| [[undefined]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be checked. I'm not quite sure why it changed from null to undefined.
|
This is awesome. Would love too see this get merged. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's amazing job! Thank you! 🥳
I made the first iteration of review and left some comments. Could you please also ensure that some changes marked in Git as renaming and not delete/add because it's hard to see the difference, especially for large files (see screenshot)
and also rename in Git will retain Git history
And also I'd prefer to apply prettier as a separate PR, but it's up to you.
|
|
||
| const ctx = { | ||
| options | ||
| } as spy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have this test-related cast here?
| name: 'MapboxDraw', | ||
| file: outputFile, | ||
| format: 'umd', | ||
| format: 'esm', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like a breaking change for customers who use this library through a script tag.
| @@ -0,0 +1 @@ | |||
| declare module 'fast-deep-equal'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @@ -0,0 +1 @@ | |||
| declare module '@mapbox/geojson-area'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @@ -0,0 +1 @@ | |||
| declare module '@mapbox/point-geometry'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "drawing" | ||
| ], | ||
| "type": "module", | ||
| "exports": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess now we can expose types for GL Draw as we do in GL JS https://github.com/mapbox/mapbox-gl-js/blob/main/package.json#L130
| @@ -1,4 +1,5 @@ | |||
| import ModeInterface from './mode_interface.js'; | |||
| import type { CTX, DrawCustomMode, StrictFeature } from '../types/types'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe use Context?
|
Interested on this been merged. |



This PR replaces
.jsfiles insrc/andtest/with.ts. Additionally:npm run tscto evaluate typingsNote: I haven't touched benchmarks. Things remain JavaScript there.
dist/mapbox-gl-draw-unminified.jsbundles at113kdist/mapbox-gl-draw.jsbundles at51k