Converts mapbox-gl-circle to typescript #86
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Upgrades Mapbox GL JS from v.0.44.1 -> v.1.6.1 Upgrades Turf v.4.7.3 -> v.5.1.6
gabrielcampos
approved these changes
Dec 4, 2020
|
Can we merge this pr please? @mblomdahl @carlba @ryanbaumann @dsperling |
mblomdahl
requested changes
Oct 14, 2022
There was a problem hiding this comment.
I love the idea of reworking this into Typescript, but I think we need to restore support for building and publishing this as NPM package, and also make sense of the thousands of changes by documenting commits before considering merging it. Please check out the commit granularity from previous feature PRs for examples of what we aimed at in the past: https://github.com/smithmicro/mapbox-gl-circle/pull/53/commits
Very nice job with the Typescript conversion @gleite! From my local testing most things seem to work very well. 🌟
Comment on lines
-88
to
-110
| window.editableCircle0 = new MapboxCircle({lat: 39.986, lng: -75.341}, 350, editableOpts).addTo(map); | ||
|
|
||
| window.plainCircle0 = new MapboxCircle({lat: 39.982, lng: -75.345}, 250, nonEditableOpts).addTo(map); | ||
|
|
||
| window.plainCircle1 = new MapboxCircle({lat: 39.983, lng: -75.344}, 300, nonEditableOpts).addTo(map); | ||
|
|
||
| window.editableCircle1 = new MapboxCircle({lat: 39.984, lng: -75.349}, 300, editableOpts).addTo(map) | ||
| .setCenter({lat: 39.989, lng: -75.348}).setRadius(50); | ||
|
|
||
| window.editableCircle2 = new MapboxCircle({lat: 39.974377, lng: -75.639449}, 25000, extraPrettyEditableOpts).addTo(map); | ||
| window.editableCircle3 = new MapboxCircle({lat: 39.980, lng: -75.340}, 225, editableOpts).addTo(map); | ||
|
|
||
| window.plainCircle2 = new MapboxCircle({lat: 39.983, lng: -75.345}, 150, nonEditableOpts).addTo(map); | ||
| window.plainCircle3 = new MapboxCircle([-75.352, 39.983], 200, nonEditableOpts).addTo(map); | ||
|
|
||
| window.setTimeout(function () { | ||
| window.editableCircle1.remove(); | ||
| window.editableCircle3.remove(); | ||
| window.setTimeout(function () { | ||
| window.editableCircle1.addTo(map).setCenter({lat: 39.984, lng: -75.349}).setRadius(300); | ||
| window.editableCircle3.addTo(map); | ||
| }, 1250); | ||
| }, 2500); |
There was a problem hiding this comment.
As I recall, these Pittsburgh centered circles was very useful during development and testing some tricky corner cases. Now, with this PR, these are eliminated in favour of the GitHub Pages circles from ./index.html centered around Stockholm (https://smithmicro.github.io/mapbox-gl-circle/). I think these clusters should be kept separately, in order to thoroughly test different scenarios.
| @@ -1,4 +1,4 @@ | |||
| Copyright (c) 2017, Smith Micro Software, Inc. | |||
| Copyright (c) 2020, Smith Micro Software, Inc. | |||
There was a problem hiding this comment.
Suggested change
| Copyright (c) 2020, Smith Micro Software, Inc. | |
| Copyright (c) 2017, Smith Micro Software, Inc. |
Citing U.S. Copyright Office:
Do I have to renew my copyright?
No. Works created on or after January 1, 1978, are not subject to renewal registration. As to works published or registered prior to January 1, 1978, renewal registration is optional after 28 years but does provide certain legal advantages. For information on how to file a renewal application as well as the legal benefit for doing so, see Circular 15, Renewal of Copyright, and Circular 15a, Duration of Copyright.
Comment on lines
9
to
+14
| COPY package.json /opt/mapbox-gl-circle/ | ||
| COPY index.html /opt/mapbox-gl-circle/ | ||
| COPY lib/main.ts /opt/mapbox-gl-circle/lib/ | ||
| COPY example/index.ts /opt/mapbox-gl-circle/example/ | ||
|
|
||
| RUN npm install && mkdir -p example/ lib/ | ||
|
|
||
| COPY lib/main.js /opt/mapbox-gl-circle/lib/ | ||
| COPY example/index.js /opt/mapbox-gl-circle/example/ | ||
| RUN npm install |
There was a problem hiding this comment.
Each of these statements will create a layer in for the container image that is subject to caching. I.e.
COPY package.json /opt/mapbox-gl-circle/ ← layer N
COPY index.html /opt/mapbox-gl-circle/ ← layer N+1
COPY lib/main.ts /opt/mapbox-gl-circle/lib/ ← layer N+2
COPY example/index.ts /opt/mapbox-gl-circle/example/ ← layer N+3
RUN npm install ← layer N+4
When you change the execution order to run the most expensive step (npm install) last, it means that any line changed in the index.html file or lib/main.ts will force a complete rebuild of "layer N+4", this is probably not what we want. So it' propose reverting to running the install step directly after adding the package.json.
Comment on lines
-67
to
-164
| #### constructor | ||
|
|
||
| **Parameters** | ||
|
|
||
| - `center` **({lat: [number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number), lng: [number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)} | \[[number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number), [number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)])** Circle center as an object or `[lng, lat]` coordinates | ||
| - `radius` **[number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)** Meter radius | ||
| - `options` **[Object](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Object)?** | ||
| - `options.editable` **[boolean](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Boolean)?** Enable handles for changing center and radius (optional, default `false`) | ||
| - `options.minRadius` **[number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)?** Minimum radius on user interaction (optional, default `10`) | ||
| - `options.maxRadius` **[number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)?** Maximum radius on user interaction (optional, default `1100000`) | ||
| - `options.strokeColor` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)?** Stroke color (optional, default `'#000000'`) | ||
| - `options.strokeWeight` **[number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)?** Stroke weight (optional, default `0.5`) | ||
| - `options.strokeOpacity` **[number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)?** Stroke opacity (optional, default `0.75`) | ||
| - `options.fillColor` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)?** Fill color (optional, default `'#FB6A4A'`) | ||
| - `options.fillOpacity` **[number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)?** Fill opacity (optional, default `0.25`) | ||
| - `options.refineStroke` **[boolean](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Boolean)?** Adjust circle polygon precision based on radius and zoom | ||
| (i.e. prettier circles at the expense of performance) (optional, default `false`) | ||
| - `options.properties` **[Object](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Object)?** Property metadata for Mapbox GL JS circle object (optional, default `{}`) | ||
|
|
||
| #### on | ||
|
|
||
| Subscribe to circle event. | ||
|
|
||
| **Parameters** | ||
|
|
||
| - `event` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)** Event name; `click`, `contextmenu`, `centerchanged` or `radiuschanged` | ||
| - `fn` **[Function](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/function)** Event handler, invoked with the target circle as first argument on | ||
| _'centerchanged'_ and _'radiuschanged'_, or a _MapMouseEvent_ on _'click'_ and _'contextmenu'_ events | ||
| - `onlyOnce` **[boolean](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Boolean)?** Remove handler after first call (optional, default `false`) | ||
|
|
||
| Returns **[MapboxCircle](#mapboxcircle)** | ||
|
|
||
| #### once | ||
|
|
||
| Alias for registering event listener with _onlyOnce=true_, see [#on](#on). | ||
|
|
||
| **Parameters** | ||
|
|
||
| - `event` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)** Event name | ||
| - `fn` **[Function](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/function)** Event handler | ||
|
|
||
| Returns **[MapboxCircle](#mapboxcircle)** | ||
|
|
||
| #### off | ||
|
|
||
| Unsubscribe to circle event. | ||
|
|
||
| **Parameters** | ||
|
|
||
| - `event` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)** Event name | ||
| - `fn` **[Function](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/function)** Handler to be removed | ||
|
|
||
| Returns **[MapboxCircle](#mapboxcircle)** | ||
|
|
||
| #### addTo | ||
|
|
||
| **Parameters** | ||
|
|
||
| - `map` **mapboxgl.Map** Target map for adding and initializing circle Mapbox GL layers/data/listeners. | ||
| - `before` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)?** Layer ID to insert the circle layers before; explicitly pass `null` to | ||
| get the circle assets appended at the end of map-layers array (optional, default `'waterway-label'`) | ||
|
|
||
| Returns **[MapboxCircle](#mapboxcircle)** | ||
|
|
||
| #### remove | ||
|
|
||
| Remove source data, layers and listeners from map. | ||
|
|
||
| Returns **[MapboxCircle](#mapboxcircle)** | ||
|
|
||
| #### getCenter | ||
|
|
||
| Returns **{lat: [number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number), lng: [number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)}** Circle center position | ||
|
|
||
| #### setCenter | ||
|
|
||
| **Parameters** | ||
|
|
||
| - `position` **{lat: [number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number), lng: [number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)}** | ||
|
|
||
| Returns **[MapboxCircle](#mapboxcircle)** | ||
|
|
||
| #### getRadius | ||
|
|
||
| Returns **[number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)** Current radius, in meters | ||
|
|
||
| #### setRadius | ||
|
|
||
| **Parameters** | ||
|
|
||
| - `newRadius` **[number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)** Meter radius | ||
|
|
||
| Returns **[MapboxCircle](#mapboxcircle)** | ||
|
|
||
| #### getBounds | ||
|
|
||
| Returns **{sw: {lat: [number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number), lng: [number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)}, ne: {lat: [number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number), lng: [number](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number)}}** Southwestern/northeastern bounds | ||
|
|
There was a problem hiding this comment.
The npm run docs step from the Update README API Documentation is broken with the changes in this PR. I see that you've written very neat and clear Typescript docs for the MapboxCricle public interface, so would it be possible to still generate a readable summary here? I looked at this TypeDoc example and it appeared promising to me.
Comment on lines
-87
to
+83
| <!--suppress ES6ConvertVarToLetConst, ES6ModulesDependencies --> | ||
| <script> | ||
| document.getElementById('version').textContent = 'mapbox-gl-circle-' + MapboxCircle.VERSION; | ||
|
|
||
| // noinspection SpellCheckingInspection | ||
| mapboxgl.accessToken = 'pk.eyJ1IjoiYW5hbmR0aGFra2VyIiwiYSI6ImNqNWptdjU1YjJheWszM256ZWN0MXNrejMifQ.TXnoKtnlam-KBmRSjXQgEw'; | ||
|
|
||
| function boundsTo5percentRadius(bounds) { | ||
| // noinspection JSUnresolvedVariable | ||
| // noinspection JSCheckFunctionSignatures | ||
| return Math.round( | ||
| turf.distance(bounds.getSouthWest().toArray(), bounds.getNorthEast().toArray(), 'meters') * .05); | ||
| } | ||
|
|
||
| var map = new mapboxgl.Map({ | ||
| container: 'map', | ||
| style: 'mapbox://styles/mapbox/streets-v9', | ||
| center: [18.059678, 59.322465], | ||
| zoom: 12.305430 | ||
| }).addControl(new mapboxgl.ScaleControl({maxWidth: 300}), 'top-right'); | ||
|
|
||
| var layerList = document.getElementById('menu'); | ||
| var inputs = layerList.getElementsByTagName('input'); | ||
|
|
||
| function switchLayer(clickEvent) { | ||
| var layerId = clickEvent.target.id; | ||
| map.setStyle('mapbox://styles/mapbox/' + layerId + '-v9'); | ||
| } | ||
|
|
||
| for (var i = 0; i < inputs.length; i++) { | ||
| inputs[i].onclick = switchLayer; | ||
| } | ||
|
|
||
| // Non-editable circles. | ||
| var lidingoMunicipality = {position: {lat: 59.379278, lng: 18.17814}, radius: 4846}; | ||
|
|
||
| new MapboxCircle(lidingoMunicipality.position, lidingoMunicipality.radius, { | ||
| strokeColor: '#0000ff', | ||
| strokeOpacity: 0.25, | ||
| strokeWeight: 2, | ||
| refineStroke: true, | ||
| fillColor: '#000000', | ||
| fillOpacity: 0.05 | ||
| }).addTo(map); | ||
|
|
||
| // Editable circles. | ||
| var northPole = {position: {lat: 84.928218, lng: -38.074812}, radius: 1100000}; | ||
| var irelandAndUK = {position: {lat: 54.431637, lng: -4.223538}, radius: 547293}; | ||
| var oldTown = {position: {lat: 59.325017, lng: 18.069263}, radius: 508}; | ||
| var smsiStockholm = {position: {lat: 59.346978, lng: 18.039120}, radius: 142}; | ||
| var kvadratSthlm = {position: {lat: 59.34131, lng: 18.058817}, radius: 122}; | ||
| var stormforsOffice = {position: {lat: 59.337678, lng: 18.083402}, radius: 85}; | ||
| var midsommarkransenSubway = {position: {lat: 59.301857, lng: 18.012292}, radius: 131}; | ||
|
|
||
| var editableCircleOpts = { | ||
| editable: true, | ||
| minRadius: 50, | ||
| debugEl: document.getElementById('debug') | ||
| }; | ||
|
|
||
| var baseDelay = 500; | ||
| [northPole, irelandAndUK, oldTown, smsiStockholm, kvadratSthlm, stormforsOffice, midsommarkransenSubway].forEach( | ||
| function (item) { | ||
| window.setTimeout(function () { | ||
| var newEditable = new MapboxCircle(item.position, item.radius, editableCircleOpts).addTo(map) | ||
| .once('click', function (mapMouseEvent) { // Left-click on circle to remove. | ||
| console.log('remove circle / mapMouseEvent:', mapMouseEvent); | ||
| newEditable.remove(); | ||
| }); | ||
| }, baseDelay += 500); | ||
| }); | ||
|
|
||
| map.on('contextmenu', function (event) { // Right-click on map to add new circle. | ||
| console.log('Add editable circle at ' + event.lngLat); | ||
| var newEditable = new MapboxCircle(event.lngLat, boundsTo5percentRadius(map.getBounds()), editableCircleOpts) | ||
| .once('click', function (mapMouseEvent) { | ||
| console.log('remove circle / mapMouseEvent:', mapMouseEvent); | ||
| newEditable.remove(); | ||
| }) | ||
| .addTo(map); | ||
| }); | ||
|
|
||
| </script> | ||
|
|
||
| <script src="bundle.js"></script> |
There was a problem hiding this comment.
This will break the GitHub Pages feature, as you are referencing a source file produced during build time and also .gitignore'd.
Comment on lines
-44
to
-47
| "engines": { | ||
| "node": ">=7.6.0", | ||
| "npm": ">=5.3.0" | ||
| }, |
There was a problem hiding this comment.
I think the engines constraint should be kept. It is useful information to keep folks in line with the current LTS (at least). I tested installing this with NPM 8 and Node versions 14, 16, and 18. No issues encountered, so let's set it to 14 ≤ Node ≤ 18 per the current release schedule.
Comment on lines
+1
to
+1380
| 'circle-opacity': 0, | ||
| 'circle-radius': horizontalPixelsPerMeter * this.radius, | ||
| 'circle-stroke-color': this.options.strokeColor, | ||
| 'circle-stroke-opacity': this.options.strokeOpacity * 0.5, | ||
| 'circle-stroke-width': this.options.strokeWeight, | ||
| }, | ||
| source: this.circleCenterHandleSourceId, | ||
| type: 'circle', | ||
| }; | ||
| } else { | ||
| return { | ||
| filter: ['==', '$type', 'Polygon'], | ||
| id: this.circleCenterHandleStrokeId, | ||
| paint: { | ||
| 'line-color': this.options.strokeColor, | ||
| 'line-opacity': this.options.strokeOpacity * 0.5, | ||
| 'line-width': this.options.strokeWeight, | ||
| }, | ||
| source: this.circleCenterHandleSourceId, | ||
| type: 'line', | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @return Style layer for the radius handles' stroke. | ||
| */ | ||
| private getRadiusHandlesStrokeLayer(): mapboxgl.Layer { | ||
| return { | ||
| filter: ['==', '$type', 'Polygon'], | ||
| id: this.circleRadiusHandlesStrokeId, | ||
| paint: { | ||
| 'line-color': this.options.strokeColor, | ||
| 'line-opacity': this.options.strokeOpacity * 0.5, | ||
| 'line-width': this.options.strokeWeight, | ||
| }, | ||
| source: this.circleRadiusHandlesSourceId, | ||
| type: 'line', | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * @return Default paint style for edit handles. | ||
| */ | ||
| private getEditHandleDefaultPaintOptions(): object { | ||
| return { | ||
| 'circle-color': '#ffffff', | ||
| 'circle-radius': 3.75, | ||
| 'circle-stroke-color': this.options.strokeColor, | ||
| 'circle-stroke-opacity': this.options.strokeOpacity, | ||
| 'circle-stroke-width': this.options.strokeWeight, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * @return Style layer for the circle's center handle | ||
| */ | ||
| private getCircleCenterHandleLayer(): mapboxgl.Layer { | ||
| return { | ||
| filter: ['==', '$type', 'Point'], | ||
| id: this.circleCenterHandleId, | ||
| paint: this.getEditHandleDefaultPaintOptions(), | ||
| source: this.circleCenterHandleSourceId, | ||
| type: 'circle', | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * @return Style layer for the circle's radius handles. | ||
| */ | ||
| private getCircleRadiusHandlesLayer(): mapboxgl.Layer { | ||
| return { | ||
| filter: ['==', '$type', 'Point'], | ||
| id: this.circleRadiusHandlesId, | ||
| paint: this.getEditHandleDefaultPaintOptions(), | ||
| source: this.circleRadiusHandlesSourceId, | ||
| type: 'circle', | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| export abstract class MapboxCircleMonostate { | ||
| public static instanceIdCounter = 0; | ||
| public static activeEditableCircles = []; | ||
| public static broadcast = new EventEmitter(); | ||
| } | ||
|
|
||
| interface Options { | ||
| editable?: boolean; | ||
| strokeColor?: string; | ||
| strokeWeight?: number; | ||
| strokeOpacity?: number; | ||
| fillColor?: string; | ||
| fillOpacity?: number; | ||
| refineStroke?: boolean; | ||
| minRadius?: number; | ||
| maxRadius?: number; | ||
| properties?: JSON; | ||
| debugEl?: any; | ||
| } |
There was a problem hiding this comment.
To me, this change is overly difficult to review. Could you please split up this commit into multiple changes so that they can be interpreted by git? As you see here, now it's so vastly different that git basically says "one giant source file removed, one (unrelated) giant source file added". Basically, we should be able to break it down into:
- Re-order object property names alphabetically per your auto-formatting tool (yields a lot of clutter)
- Change naming convention for the event handlers from
/** @private */ _aPrivateMethod() {...}→private aPrivateMethod() {...}(also adds a lot of clutter) - Move type hints from JSDoc to Typescript,
/** @param {string} event */→event: string(more clutter) - Re-order the methods of
MapboxCircleclass (more clutter) - Additional functional changes (if any?!)
package.jsonandtsconfig.jsonconfig updates with some notes on how the new script commands are intended to function
This was referenced Oct 21, 2022
2 tasks
|
This will be replaced by the typing effort on #103. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Upgrades Mapbox GL JS from v.0.44.1 -> v.1.6.1
Upgrades Turf v.4.7.3 -> v.5.1.6