Fix start-debug to watch ts files #704

jazzzz · 2021-12-10T16:21:30Z

This PR fixes start-debug and the watch scripts.

Launch Checklist

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.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality.
Document any changes to public APIs.
Post benchmark scores.
Manually test the debug page.
Suggest a changelog category: bug/feature/docs/etc. or "skip changelog".
Add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>.

HarelM · 2021-12-10T20:30:10Z

It's great that you managed to make the typescript rollup plugin work, I came across some nasty errors when I did the typescript migration.
Note that some typescript configuration in the tsconfig are needed for the types generating and not the tsc command itself, so make sure the bundled typings are mostly the same.
I think the rollup_typescript code should be in the rollup plugins file like the rest of the plugin configuration, wouldn't it be cleaner?
Other than that, great direction!

jazzzz · 2021-12-10T20:42:28Z

It's great that you managed to make the typescript rollup plugin work, I came across some nasty errors when I did the typescript migration.
Note that some typescript configuration in the tsconfig are needed for the types generating and not the tsc command itself, so make sure the bundled typings are mostly the same.

maplibre-gl.d.ts did not change, did you think about something else?

I think the rollup_typescript code should be in the rollup plugins file like the rest of the plugin configuration, wouldn't it be cleaner?

I did it first, but I extracted it to use it for build-type-spec that does not use the rollup plugins file, but I could make it use this file.

HarelM · 2021-12-10T20:53:07Z

Yes, I meant maplibre-gl.d.ts. Did you delete the dist folder to make sure it's generated? Sorry for nagging, I just fought with it a lot to make it run so I'm surprised all these configurations are not actually needed...

Also note that I let the tests run and some are failing, you'll need to investigate why, might be related to the bundle size change as some things broke... IDK...
Thanks again!!

jazzzz · 2021-12-10T21:02:46Z

Yes, I meant maplibre-gl.d.ts. Did you delete the dist folder to make sure it's generated? Sorry for nagging, I just fought with it a lot to make it run so I'm surprised all these configurations are not actually needed...

Yes I moved the folder between each commit to make sure to compare them. In fact it's still dts-bundle-generator that builds the types bundle, this explains that it is exactly the same, and I don't build it in the watch script because I don't think it is useful in this mode.

Also note that I let the tests run and some are failing, you'll need to investigate why, might be related to the bundle size change as some things broke... IDK... Thanks again!!

I'm still not that familiar with all the tests,, but I'm investigating to make all the tests pass. Thanks for the support!

github-actions · 2021-12-16T17:49:15Z

Bundle size report:

Size Change: 0 B
Total Size Before: 194 kB
Total Size After: 194 kB

Output file	Before	After	Change
maplibre-gl.js	185 kB	185 kB	0 B
maplibre-gl.css	9.49 kB	9.49 kB	0 B

ℹ️ View Details

No major changes

jazzzz · 2021-12-16T17:58:14Z

I changed my approach, now the build are nearly the same as before, thus the generated bundles are exactly the same, I only use the typescript rollup plugin in watch mode.

HarelM · 2021-12-16T18:25:48Z

Thanks for looking into it! Very much appreciated!
Doesn't this mean that the output of the watch is not valid?
If it is valid we can use it instead of the current build, can't we?, If it's not valid then what is the use case?

jazzzz · 2021-12-16T18:35:58Z

Doesn't this mean that the output of the watch is not valid?
If it is valid we can use it instead of the current build, can't we?, If it's not valid then what is the use case?

The output is valid, but many tests use the intermediate js files that the watch do not generate. One way would be to use ts files in the tests, but I could not make it work.

wipfli · 2021-12-16T18:38:06Z

We are migrating the unit tests to Jest, which can parse the TypeScript files directly. Or are you talking about different tests?

HarelM · 2021-12-16T18:41:56Z

But I saw that the build tests failed, which I think indicates that something is a bit off with the output...?
I think that if the build can use the typescript rollup plugin it's better then now, but we should finish the migration of the tests before that.
IDK, maybe we can use this PR as a starting point for using the rollup plugin.

jazzzz · 2021-12-17T11:06:21Z

We are migrating the unit tests to Jest, which can parse the TypeScript files directly. Or are you talking about different tests?

Tap can parse TypeScript files, but I had issues when mixing js and ts files. I could solve it for some tests by converting them fully to TypeScript, but some tests like style-spec.test.js include files that cannot be converted to TypeScript. Also browser tests needs some files (like map.ts) to be transpiled to run them in the browser, and I could not make them work.

But I saw that the build tests failed, which I think indicates that something is a bit off with the output...?

Maybe you saw the tests of an old push, now the tests succeed, or maybe you are talking about something else?

HarelM · 2021-12-17T15:25:08Z

Any chance you can create another PR with the typescript rollup plugin "always on" so we can look at the failing tests to make sure they are only related to js tap tests and not the output of the rollup build (i.e. the build tests)?

jazzzz · 2021-12-17T15:32:27Z

Any chance you can create another PR with the typescript rollup plugin "always on" so we can look at the failing tests to make sure they are only related to js tap tests and not the output of the rollup build (i.e. the build tests)?

If you want, but most tests won't even start because they try to import js files in rollup/build/tsc that are not generated with the typescript rollup plugin.

HarelM · 2021-12-17T15:45:59Z

I know, I know, that's fine...

jazzzz · 2021-12-17T15:53:09Z

Any chance you can create another PR with the typescript rollup plugin "always on" so we can look at the failing tests to make sure they are only related to js tap tests and not the output of the rollup build (i.e. the build tests)?

PR #720.

HarelM

nits...

build/rollup_plugins.js

tsconfig.json

build/glsl_to_js.js

HarelM · 2021-12-21T20:29:23Z

Let's merge this. Hopefully we'll be able to fully integrate typescript into rollup later on, but right now this at least fixes the watch scripts.
Truly appreciate all the time invested in this @jazzzz!

* main: (98 commits) [Jest] Migrate `geojson_worker_source.test.js` (maplibre#731) Fix events being fired after Map#remove has been called when the WebGL context is lost and restored (maplibre#726) (maplibre#727) Define return type of getSource as possibly undefined (maplibre#724) Fix attibution controll (maplibre#668) Fix start-debug to watch ts files (maplibre#704) [Jest] Migrate `touch_zoom_rotate.test.js` (maplibre#721) [Jest] Migrate `requestRenderFrame.test.js` (maplibre#722) [Jest] Migrate `scroll_zoom.test.js` (maplibre#712) [Jest] Migrate `marker.test.js` (maplibre#696) [Jest] Migrate `mouse_rotate.test.js` (maplibre#711) [Jest] Migrate `keyboard.test.js` (maplibre#707) [Jest] Migrate `map_event.test.js` (maplibre#710) [Jest] Migrate `drag_rotate.test.js` (maplibre#709) Handle spies and call counts (maplibre#708) [Jest] Migrate `drag_pan.test.js` (maplibre#702) Add type for styleimagemissing event (maplibre#703) Fix MapDataEvent#isSourceLoaded being true in GeoJSONSource "dataloading" event handlers (maplibre#694) (maplibre#695) [Jest] Migrate `dblclick_zoom.test.js` (maplibre#697) [Jest] Migrate `popup.test.js` (maplibre#687) Fixed typo (maplibre#698) ...

* pluggable-render: (102 commits) WIP - pluggable render update Removed a badly typed variable [Jest] Migrate `geojson_worker_source.test.js` (maplibre#731) Fix events being fired after Map#remove has been called when the WebGL context is lost and restored (maplibre#726) (maplibre#727) Define return type of getSource as possibly undefined (maplibre#724) Fix attibution controll (maplibre#668) Fix start-debug to watch ts files (maplibre#704) Filter out hillshade layers Filter out hillshade styles [Jest] Migrate `touch_zoom_rotate.test.js` (maplibre#721) [Jest] Migrate `requestRenderFrame.test.js` (maplibre#722) [Jest] Migrate `scroll_zoom.test.js` (maplibre#712) [Jest] Migrate `marker.test.js` (maplibre#696) [Jest] Migrate `mouse_rotate.test.js` (maplibre#711) [Jest] Migrate `keyboard.test.js` (maplibre#707) [Jest] Migrate `map_event.test.js` (maplibre#710) [Jest] Migrate `drag_rotate.test.js` (maplibre#709) Handle spies and call counts (maplibre#708) [Jest] Migrate `drag_pan.test.js` (maplibre#702) Add type for styleimagemissing event (maplibre#703) ...

jazzzz mentioned this pull request Dec 10, 2021

Remove watch scripts from package.json #445

Closed

jazzzz force-pushed the watch branch 3 times, most recently from fceba56 to df6666d Compare December 10, 2021 19:13

jazzzz force-pushed the watch branch from df6666d to 6666d21 Compare December 16, 2021 17:41

jazzzz force-pushed the watch branch from 6666d21 to 118722b Compare December 16, 2021 17:57

jazzzz force-pushed the watch branch from 118722b to 8ad5d43 Compare December 16, 2021 18:09

jazzzz mentioned this pull request Dec 17, 2021

Use rollup typescript plugin to build the bundles #720

Closed

9 tasks

HarelM requested changes Dec 20, 2021

View reviewed changes

build/rollup_plugins.js Outdated Show resolved Hide resolved

tsconfig.json Outdated Show resolved Hide resolved

build/glsl_to_js.js Outdated Show resolved Hide resolved

jazzzz force-pushed the watch branch from 8ad5d43 to 0218a3a Compare December 21, 2021 11:15

Fix start-debug to watch ts files

5aa481f

jazzzz force-pushed the watch branch from 0218a3a to 5aa481f Compare December 21, 2021 14:18

HarelM approved these changes Dec 21, 2021

View reviewed changes

HarelM merged commit d8f74e8 into maplibre:main Dec 21, 2021

ZeLonewolf mentioned this pull request Jan 2, 2022

Shield rotation #716

Merged

9 tasks

HarelM mentioned this pull request Feb 10, 2022

WorkerTransfer benchmark 3x slower on main compared to v2.1.1 #981

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix start-debug to watch ts files #704

Fix start-debug to watch ts files #704

jazzzz commented Dec 10, 2021 •

edited

Loading

HarelM commented Dec 10, 2021

jazzzz commented Dec 10, 2021 •

edited

Loading

HarelM commented Dec 10, 2021

jazzzz commented Dec 10, 2021

github-actions bot commented Dec 16, 2021 •

edited

Loading

jazzzz commented Dec 16, 2021

HarelM commented Dec 16, 2021

jazzzz commented Dec 16, 2021

wipfli commented Dec 16, 2021

HarelM commented Dec 16, 2021

jazzzz commented Dec 17, 2021

HarelM commented Dec 17, 2021

jazzzz commented Dec 17, 2021

HarelM commented Dec 17, 2021

jazzzz commented Dec 17, 2021

HarelM left a comment

HarelM commented Dec 21, 2021

Fix start-debug to watch ts files #704

Fix start-debug to watch ts files #704

Conversation

jazzzz commented Dec 10, 2021 • edited Loading

Launch Checklist

HarelM commented Dec 10, 2021

jazzzz commented Dec 10, 2021 • edited Loading

HarelM commented Dec 10, 2021

jazzzz commented Dec 10, 2021

github-actions bot commented Dec 16, 2021 • edited Loading

jazzzz commented Dec 16, 2021

HarelM commented Dec 16, 2021

jazzzz commented Dec 16, 2021

wipfli commented Dec 16, 2021

HarelM commented Dec 16, 2021

jazzzz commented Dec 17, 2021

HarelM commented Dec 17, 2021

jazzzz commented Dec 17, 2021

HarelM commented Dec 17, 2021

jazzzz commented Dec 17, 2021

HarelM left a comment

Choose a reason for hiding this comment

HarelM commented Dec 21, 2021

jazzzz commented Dec 10, 2021 •

edited

Loading

jazzzz commented Dec 10, 2021 •

edited

Loading

github-actions bot commented Dec 16, 2021 •

edited

Loading