Skip to content
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

Merged
merged 15 commits into from
Dec 8, 2024
Merged

CJS to ESM #7311

merged 15 commits into from
Dec 8, 2024

Conversation

na9da
Copy link
Collaborator

@na9da na9da commented Nov 14, 2024

What this PR does

Convert CJS modules to ESM.

Required for #6998
Depends on: https://github.com/terriajs/terriamap/tree/cjs-to-esm

TODO:

  • Test node build

What this PR does:

  • Removes stale CJS files that are no longer used (395d3b9)

  • Convert CJS style module.exports to ESM style export default ... (7129b24)

  • Convert CJS require() to ESM import statements. (30654f1)
    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.

  • Remove use strict declaration for .tsx? files.

Test me

Test http://ci.terria.io/cjs-to-esm

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

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.
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@@ -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;
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

@pjonsson
Copy link
Contributor

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.

@na9da na9da marked this pull request as ready for review December 2, 2024 06:26
@na9da na9da mentioned this pull request Dec 5, 2024
7 tasks
na9da added 2 commits December 6, 2024 10:51
This was previously used for loading polyfills from polyfill.io then we switched to core-js.
@na9da
Copy link
Collaborator Author

na9da commented Dec 6, 2024

@ljowen - Sorry I forgot, but this also requires approving the terriamap PR TerriaJS/TerriaMap#720. The main change there is removing the call to polyfill() which basically does nothing. It previously used to import polyfills from polyfill.io but was replaced with core-js.

@na9da
Copy link
Collaborator Author

na9da commented Dec 8, 2024

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.
@na9da na9da merged commit 619ec17 into main Dec 8, 2024
9 checks passed
@na9da na9da deleted the cjs-to-esm branch December 8, 2024 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants