-
Notifications
You must be signed in to change notification settings - Fork 365
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
CJS to ESM #7311
CJS to ESM #7311
Conversation
These are files from TerriaJS V7 not referenced anywhere else. Some of these might be useful if we want to port them to v8 in the future. If that's the case please refer to https://github.com/terriajs/terriajs/tree/terriajs7
This requires, in some places: - simply swapping require with import - creating a stub .d.ts module definition in lib/ThirdParty - updating the library to get new bundled type definitions - file-saver - react-responsive - or adding @types/x as dependency - @types/proj4 Remaining require() imports are all for static assets or web workers which can be fixed when upgrading to webpack 5.
It is redundant when using typescript.
@@ -31,7 +31,7 @@ export const TourExplanationBox = styled(Box)` | |||
h1, | |||
h2, | |||
h3 { | |||
margin-bottom: ${(p) => p.theme.spacing * 3}px; | |||
margin-bottom: ${(p) => (p.theme.spacing as any) * 3}px; |
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.
Can spacing be as number
instead of any, or is that wrong?
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.
That will require casting to unknown
first - eg p.theme.spacing as unknown as number
Conversion of type 'string' to type 'number' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
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.
Yeah, probably better to leave it as any then, so it's easy to find in the future.
I'm not qualified to comment on the removed files, but I found a few minor nits that I commented about, The rest of this PR is "just" mechanical changes and they look consistent to me, so they are either all wrong or they are all right. Considering how many files this PR touches, I think it would be great to get this merged soon. |
This was previously used for loading polyfills from polyfill.io then we switched to core-js.
@ljowen - Sorry I forgot, but this also requires approving the terriamap PR TerriaJS/TerriaMap#720. The main change there is removing the call to |
Reverted change removing polyfill.js to avoid breaking change in terriamap. It is better to lump this change with webpack 5 upgrade which will have several breaking changes. |
To avoid breaking change to terriamap. We'll make this change when upgrading to webpack5.
What this PR does
Convert CJS modules to ESM.
Required for #6998
Depends on: https://github.com/terriajs/terriamap/tree/cjs-to-esm
TODO:
What this PR does:
Removes stale CJS files that are no longer used (395d3b9)
Convert CJS style
module.exports
to ESM styleexport default ...
(7129b24)Convert CJS require() to ESM import statements. (30654f1)
This requires, in some places:
file-saver
react-responsive
@types/proj4
Remaining require() imports are all for static assets or web workers which can be fixed when upgrading to webpack 5.
Remove
use strict
declaration for.tsx?
files.Test me
Test http://ci.terria.io/cjs-to-esm
Checklist
doc/
.