-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Chore] fix tests #2602
[Chore] fix tests #2602
Conversation
heshan0131
commented
Aug 8, 2024
•
edited
Loading
edited
- Fix jest test by importing loader.gl/polyfill from esm
- Fix browser jsdom test by importing loaders.gl/polyfill from esm
- Import @kepler.gl src in test
- delete unused files
- Update jsdom
✅ Deploy Preview for keplergl2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
'@mapbox/tiny-sdf': `${nodeModules}/@mapbox/tiny-sdf/index.cjs`, | ||
// compile from @kepler.gl src | ||
...RESOLVE_ALIASES, | ||
// loaders.gl cjs bundle of polyfills is not transpiled properly, use esm instead |
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.
Unless there is a bug (in which case we should report and fix it), "not transpiled properly" is probably an exaggeration, (though I understand it can feel that way when fighting through a number of build issues).
AFAIK we use conservatively chosen, highly compatible transpilation targets. But we do not transpile to pure ES5. And I believe that this is similar to e.g. mapbox.
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.
Admittedly I did not look into why the cjs build failed when installing @loaders.gl/polyfill
in kepler. I just want to make the test run, and importing the ESM module is the cleanest way without changing our test.
What I observe is:
@loaders.gl/polyfill@4.2.2
is installed- The cjs build in dist is missing
installFilePolyfills
implementation, see image below.

This is not necessarily a loader.gl issue. There could be multiple reason for this. We are using yarn 1.22 and node 18 to build kepler locally, upgrading both could help. I will revisit this issue once we continue to upgrade our build enviroment
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.
LGTM
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
14e9612
to
6bc7004
Compare