Simplify build pipeline #961
Merged
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
|
Bundle size report: Size Change: +441 B
ℹ️ View Details
|
|
Can we migrate totally to typescript plugin? Where are we still using the typescript output files? |
|
Also, since we have a build folder I would consider moving the rollup config there and move the bench folder under test folder? |
I'm unfortunately not sure I completely understand this question. If you ask if we still have references pointing to /rollup/build/tsc/src rather than /src, then the answer is that all import statements already has been updated to use the ts files in /src directly. That situation is not changed by this PR. |
|
Great! This means we can stop using the extra step of build-tsc and simply remove the watch flag from rollup config to always use the typescript plugin. |
|
Just to understand the background - How come rollup only used the typescript() plugin when in watch mode? |
|
It was introduced after the migration was finished as I thought that there are issues with it when I did the migration. |
|
I have tried to remove the watch() from the rollup config, but there are a few things that came up. We have this line in package.json, that I'm not sure when is executed.. and with the rollup/build/tsc folder gone, it doesn't make a lot of sense as is. |
|
Also it was easier to make the tests pass this way instead if forcing a full migration like we are in now. So it allowed us to do it one step at a time. |
|
Ahh, yes, the last part of the build simplification that I didn't manage to solve. |
|
The way it should be as I see it, is that the new Worker code should be in src and there should be a hook to tell jest to use a worker mock like the implementation. |
|
It took me a while to understand that there's a rollup plugin that uses a property defined in the package.json file to replace a file while building. |
|
How do we test that this web worker replacement work? |
|
Not sure, maybe open the dev build artifacts in the browser and see that the code is not there (using npm run start-debug)? |
HarelM
reviewed
Feb 6, 2022
HarelM
reviewed
Feb 6, 2022
HarelM
reviewed
Feb 6, 2022
HarelM
reviewed
Feb 6, 2022
|
Web worker issue: Previously, when running start-debug, watch mode would be active, and the typescript() plugin would be used in rollup loading files directly from /src. The browser replacement specified in package.json would not work, as there would be no rollup/build/tsc/src folder. In case build-tsc had been ran previously to this, it would have generated the rollup/build/tsc/src, and this would suddenly work. Now, we don't have the rollup/build/tsc/src anymore, so this path would never work. I have tried to update the path towards src instead, from: "./rollup/build/tsc/src/util/web_worker.js", to:
... and more combinations, but nothing happens. I rolled back to master, and see the same, so it could very well be the case that this "browser" trick never worked with the typescript plugin in rollup. I have inspected the /web_worker_replacement.js, and realized that it doesn't fulfill the requirements of src/source/worker.ts that tries to import Maybe something like this could be a way to avoid this replacement altogether: https://github.com/developit/web-worker, but there are some typing warnings currently. 1, 2. |
|
Another option is to use jsdom-worker, and mock jest, leaving the source code clean. |
|
It would be great if we could avoid the replacement totally. I hope the referenced package can do that. If so it would be my preferred solution to remove the replacement and delete this extra code. |
|
I tried jsdom-worker, jsdom-worker-fix, and web-worker, and I didn't manage to fix this. Do we strictly need this web worker? It seems like I can run the map just fine without this script being replaced. Or is it because the code added for node, maybe can run in the browser too? |
Merged
birkskyum
force-pushed
the
move-rollup-config-to-typescript
branch
from
e46ff8d to
805f65f
Compare
birkskyum
changed the title
birkskyum
changed the title
birkskyum
force-pushed
the
move-rollup-config-to-typescript
branch
2 times, most recently
from
9800935 to
07392ab
Compare
birkskyum
force-pushed
the
move-rollup-config-to-typescript
branch
from
36d4582 to
4c408e7
Compare
birkskyum
force-pushed
the
move-rollup-config-to-typescript
branch
from
4c408e7 to
2958296
Compare
|
COOL!!! Nice work mate! Including the web workers, right? |
HarelM
approved these changes
Feb 9, 2022
Including the web workers! |
|
So we are fully typescript end to end, right? |
|
The render tests are not on jest yet - still on harness as the last piece, but they will get there. I tried to move stub_loader (used in render test only) to ts, but it turned out to be breaking change, so that file is still .js, but we are pretty much typescript throughout. |
wipfli
added a commit
that referenced
this pull request
Feb 24, 2022
* Migrate expression tests to jest (#965) * Revert "Move benchmarks to ES modules (#964)" (#969) This reverts commit aa8ed9d. * Migrate query tests from puppeteer to playwright (#966) * Fix web worker in watch mode (#968) * Fix web worker in watch mode * Add webworker support throughout * Don't change rollup for style-spec and benchmarks * Revert "Don't change rollup for style-spec and benchmarks" This reverts commit 62a7e14. * Remove duplicate node-resolve configuration * Simplify build pipeline (#961) * Remove build-tsc compile step * re-enable failing style-spec test * cleanup * cleanup * fix style-spec-test * Create codeql-analysis.yml * Bump simple-get from 3.1.0 to 3.1.1 (#971) Bumps [simple-get](https://github.com/feross/simple-get) from 3.1.0 to 3.1.1. - [Release notes](https://github.com/feross/simple-get/releases) - [Commits](feross/simple-get@v3.1.0...v3.1.1) --- updated-dependencies: - dependency-name: simple-get dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump nanoid from 3.1.30 to 3.2.0 (#973) Bumps [nanoid](https://github.com/ai/nanoid) from 3.1.30 to 3.2.0. - [Release notes](https://github.com/ai/nanoid/releases) - [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md) - [Commits](ai/nanoid@3.1.30...3.2.0) --- updated-dependencies: - dependency-name: nanoid dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump cached-path-relative from 1.0.2 to 1.1.0 (#972) Bumps [cached-path-relative](https://github.com/ashaffer/cached-path-relative) from 1.0.2 to 1.1.0. - [Release notes](https://github.com/ashaffer/cached-path-relative/releases) - [Commits](https://github.com/ashaffer/cached-path-relative/commits) --- updated-dependencies: - dependency-name: cached-path-relative dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Move browser test to playwright (#974) * Bump ts-node for 3x faster postinstall (#975) * Remove web worker replacement (#976) * Attribution default open for osm (#795) * Update CHANGELOG.md * Revert "Update CHANGELOG.md" This reverts commit 0b81a41. * attribution fixes (from astridx) a1272d9 b9b0370 * Update .gitignore * fix missing new line lint complained about * Revert "attribution fixes (from astridx)" This reverts commit 2031d8e. * Default compact to be open by default Default compact button to be open by default, to make OpenStreetMap attibutions to show by default like mentioned in ( #205 ) This uses an idea from ( #205 (comment) ) to show compact open by default, but close it when dragged. * remove test string I forgot to remove * fix compact button showing when there are no attributions * update whitespace trim to es6 syntax * fixes after testing various states of compact * revert back so it is open when starting in fullscreen * revert .gitignore * fix lint errors * fix tests Most of these fails because I made the compact button not show if attributes are empty. The other test fail because when switching between >640 and <=640 the open attribute is now added * lint * Update package-lock.json * Update package-lock.json * Update CHANGELOG.md * delete problematic files * put back deleted files * test/integration/assets/sprites/icon-text-fit-1x@2x.json * restore files Co-authored-by: acalcutt <acalcutt@worcester.edu> * Fix benchmarks (#984) * Move bench under `test` folder (#979) * Add typeof guard to performance variable (#986) * Fixes #768 * Fix comment and add changelog * Release 2.1.2 Version (#987) * Release 2.1.2 Version * Fix changelog according to comments * Fix release.yml due to bench folder move (#988) * lint function-url-quotes/ (#983) * Bump to v2.1.3 - Fix postinstall error `ts-node` not found in non-dev installs (#991) * Bump to v2.1.4 - Fix missing `postinstall.js` file in npm publish (#992) * Bump to v2.1.5-pre.1 - Publish empty `postinstall.js` file (#994) * Bump to v2.1.5 (#996) * Bump to v2.1.6-pre.1 - Fix `dist/package.json` (#998) * Bump to v2.1.6 (#999) * prefix (#1004) * correct done (#1006) * Add adjustment to rendering glyphs (#1005) * add topAdjustment for glyph * add topAdjustment for glyph * update changelog * add issue number * add alphabet and cjk text test #1002 * fix lint error * Simplify render tests (#1003) Fixes #1008 * Initial commit - half of the tests are failing * Missing file * Remove template rendering * Fix usage of localizeUrls * Move harness and server to render folder, simplify render code to be sepcific. * Move function outside and add some typings * lint * Add typings, move compare results outside * Remove server usage and replace it by file system reading. * Move xhr mocking to main flow * Move functions outside the render function * Move creation of tests to render.ts, added more typings * Move suite implementation into main flow file, add some typings. * lint * Moved all logic to a single file that runs the render tests * remove color from harness * Remove jest run command * Small fixes * lint * Cleanup * More cleanup * Remove harness and move it to a single file. Remove the jest version. * Remove ignore related code and tests * Remove ignore, add types * Add more types * lint * Migrate actual image generation to not use Buffer * lint * Fix CI run * Fix CI properly this time... * Revert png change * Revert only buffer usage * Revert png and buffer * Simplify png in mock file * Move to use uint8array and change throshold * Added diff info * remove specific threshold from test as it fails * Fix typo * Fix according to code review * Husky pre-commit do not fix lint (#1019) * Fix css * Use main package-lock.json Co-authored-by: Birk Skyum <74932975+birkskyum@users.noreply.github.com> Co-authored-by: Yuri Astrakhan <yuriastrakhan@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Harel M <harel.mazor@gmail.com> Co-authored-by: Andrew Calcutt <acalcutt@techidiots.net> Co-authored-by: acalcutt <acalcutt@worcester.edu> Co-authored-by: vanilla-lake <95251033+vanilla-lake@users.noreply.github.com> Co-authored-by: Astrid <astridx@users.noreply.github.com> Co-authored-by: Kanahiro Iguchi <mediterranean1769@gmail.com>
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.
We can remove the manual precompilation step (build-tsc) that generated /rollup/build/tsc. Blocked by #968