Typescript: TMP WIP further fixes including window and point.ts (rebase on typescript gl matrix) #224

chippieTV · 2021-07-25T00:34:49Z

confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
briefly describe the changes in this PR

Just sharing progress, individual groups of fixes can be cherry picked out of this into PRs

down to 202 errors (from 258.. - 56 errors)
rebased on gl-matrix which we are not 100% confident to merge yet
uses point.ts with local imports and types instead of point-geometry library
remove references to window where appropriate (window is imported so it can be overridden for testing, be careful we can still test all we need to)
apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>

…ride

HarelM · 2021-07-25T05:57:28Z

I'll review each commit separately and let you know which commit I think should be picked to a separate PR so it can be merged.
I'll add my review in each commit I guess.

flow-typed/gl.js

src/types/non-typed-modules.d.ts

src/types/window.ts

src/util/browser.ts

src/util/dom.ts

HarelM · 2021-07-25T06:05:53Z

Let's create a PR with this commit after the changes:
2c709a0

HarelM · 2021-07-25T06:07:05Z

These one too:
6faf014
a5ddac3
62eb71a
d2fd869

HarelM · 2021-07-25T06:08:18Z

src/util/performance.ts

-    load: 'load',
-    fullLoad: 'fullLoad'
-};
+export enum PerformanceMarkers {


That's a much better solution than I had... Cool

HarelM · 2021-07-25T06:09:25Z

src/ui/handler/scroll_zoom.ts

@@ -151,7 +150,7 @@ class ScrollZoomHandler {
        if (!this.isEnabled()) return;

        // Remove `any` cast when https://github.com/facebook/flow/issues/4879 is fixed.
-        let value = e.deltaMode === (window.WheelEvent as any).DOM_DELTA_LINE ? e.deltaY * 40 : e.deltaY;
+        let value = e.deltaMode === (WheelEvent as any).DOM_DELTA_LINE ? e.deltaY * 40 : e.deltaY;


I think there's no need for as any here as typescript is supporting this? also the comment above is probably not needed too...

removed in new PR

HarelM · 2021-07-25T06:10:52Z

src/util/browser/web_worker.ts

 import mapboxgl from '../../';

 import type {WorkerInterface} from '../web_worker';

 export default function(): WorkerInterface {
-    return new window.Worker(mapboxgl.workerUrl) as any;
+    return new Worker(mapboxgl.workerUrl) as any;


Can we change this to maplibregl (rename of the import veriable)?

sure, didn't notice.. I will keep an eye out for Mapbox references in future. I think I've seen a few and wondered why they were there but assumed compatibility with existing data sources or something. Maybe a project wide search to find all and document which ones we want to keep and why so we don't keep running into this.

I tried to did a full search, I fixed most of it, but didn't want to change things related to workers.
But don't invest time in this right now, we'll do it later on when everything starts working again...

HarelM · 2021-07-25T06:11:32Z

src/util/tile_request_cache.ts

@@ -120,7 +120,7 @@ export function cacheGet(request: Request, callback: (error?: any | null, respon

 function isFresh(response) {
    if (!response) return false;
-    const expires = new Date(response.headers.get('Expires') || 0);
+    const expires = new Date(response.headers.get('Expires') || 0).valueOf();


Shouldn't this be getTime()?

https://stackoverflow.com/questions/9710136/in-javascript-why-do-date-objects-have-both-valueof-and-gettime-methods-if-they

To be honest I've never used get time but I don't know that there are cases where they might behave differently. I think you're right, something from the SO post makes me believe that getTime() would always return the same while maybe different browsers could have some different internal representation? I'm fine to update to getTime.. will make the change in the cherry-picked PR

HarelM · 2021-07-25T06:18:40Z

OK, I reviewed the commits.
I have done similar code changes so this shows that the solution is probably the same.
Above are some minor comments and fixes requests - mainly to remove comments and delete files...
Can we move the code changes related to glMatrix to a different branch so once we have the tests running we could see that it doesn't break anything and currently only do some typings workarounds as such as any as...?
I would like to have a working version with minimal code changes and only then start removing awkward typings. what do you say?
I think we are very close to finish fixing the typings, good work!

chippieTV · 2021-07-25T06:55:12Z

I know we don't want to merge gl-matrix yet but to avoid re-fixing the same issues (or just looking at the noise rather than actual unsolved stuff) I worked on top of the unmerged branch, with the aim to cherry pick, so thanks for the review, I'll get those broken out into PRs.

chippieTV · 2021-07-26T04:38:26Z

this is no longer useful, closing
please refer to gl-matrix PR independently instead
#219

chippieTV added 13 commits July 25, 2021 09:30

update use of mat4

cc879b0

updating types as gl-matrix

8221087

more types to gl-matrix

4c29cf9

cleanup cast vs constructor

0af4b74

cleanup more gl-matrix types

a4c5325

more using gl-matrix instead of Float32Array

012da5a

WIP mess a lot of stuff up - separate into sensible PRs

2c709a0

fix cannot find type Point errors

6faf014

add static to fix convert does not exist on typeof Point errors

1642415

cast type of else statement - might work better with type guard

a5ddac3

convert object to enum

62eb71a

removing references to window - careful if testing, jsdom cannot over…

d2fd869

…ride

fix mat4 type on custom layer - keep an eye on this in testing

bed0ec0

HarelM reviewed Jul 25, 2021

View reviewed changes

flow-typed/gl.js Show resolved Hide resolved

src/types/non-typed-modules.d.ts Show resolved Hide resolved

src/types/window.ts Show resolved Hide resolved

src/util/browser.ts Show resolved Hide resolved

src/util/dom.ts Show resolved Hide resolved

HarelM reviewed Jul 25, 2021

View reviewed changes

chippieTV closed this Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typescript: TMP WIP further fixes including window and point.ts (rebase on typescript gl matrix) #224

Typescript: TMP WIP further fixes including window and point.ts (rebase on typescript gl matrix) #224

chippieTV commented Jul 25, 2021

HarelM commented Jul 25, 2021

HarelM commented Jul 25, 2021

HarelM commented Jul 25, 2021 •

edited

Loading

HarelM Jul 25, 2021

HarelM Jul 25, 2021

chippieTV Jul 25, 2021

HarelM Jul 25, 2021

chippieTV Jul 25, 2021

HarelM Jul 25, 2021

HarelM Jul 25, 2021

chippieTV Jul 25, 2021

HarelM commented Jul 25, 2021

chippieTV commented Jul 25, 2021

chippieTV commented Jul 26, 2021 •

edited

Loading

Typescript: TMP WIP further fixes including window and point.ts (rebase on typescript gl matrix) #224

Typescript: TMP WIP further fixes including window and point.ts (rebase on typescript gl matrix) #224

Conversation

chippieTV commented Jul 25, 2021

HarelM commented Jul 25, 2021

HarelM commented Jul 25, 2021

HarelM commented Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarelM commented Jul 25, 2021

chippieTV commented Jul 25, 2021

chippieTV commented Jul 26, 2021 • edited Loading

HarelM commented Jul 25, 2021 •

edited

Loading

chippieTV commented Jul 26, 2021 •

edited

Loading