Skip to content

CJS to ESM#7311

Merged
na9da merged 15 commits into
mainfrom
cjs-to-esm
Dec 8, 2024
Merged

CJS to ESM#7311
na9da merged 15 commits into
mainfrom
cjs-to-esm

Conversation

@na9da

@na9da na9da commented Nov 14, 2024

Copy link
Copy Markdown
Collaborator

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.
Comment thread package.json Outdated
Comment thread package.json
Comment thread lib/Core/ConsoleAnalytics.js Outdated
Comment thread lib/Models/Catalog/Esri/ArcGisFeatureServerCatalogItem.ts Outdated
Comment thread lib/Models/Catalog/Esri/ArcGisFeatureServerCatalogItem.ts Outdated
Comment thread lib/ReactViews/Map/Panels/SharePanel/SharePanel.tsx
h2,
h3 {
margin-bottom: ${(p) => p.theme.spacing * 3}px;
margin-bottom: ${(p) => (p.theme.spacing as any) * 3}px;

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Comment thread lib/Models/Catalog/Esri/ArcGisFeatureServerCatalogItem.ts Outdated
Comment thread lib/Models/Catalog/Esri/ArcGisMapServerCatalogItem.ts Outdated
@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

na9da commented Dec 6, 2024

Copy link
Copy Markdown
Collaborator Author

@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

na9da commented Dec 8, 2024

Copy link
Copy Markdown
Collaborator Author

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
@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