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

modern esm support #438

Merged
merged 39 commits into from
Sep 25, 2024
Merged

modern esm support #438

merged 39 commits into from
Sep 25, 2024

Conversation

axe312ger
Copy link
Contributor

@axe312ger axe312ger commented Feb 19, 2024

This PR, building upon #437, introduces comprehensive and up-to-date ESM support for the SDK-CORE, CMA, and CDA libraries.

Our goal is to transition these user-facing libraries to dual packages (both CommonJS and ESM compatible) as outlined in the Node.js documentation, with an added browser bundle for broader accessibility.

The SDK core will adopt an ESM-only format to guarantee compatibility with ESM-only dependencies, catering to our diverse range of customers by ensuring optimal usability across various JavaScript environments.

Complementary PRs for CDA and CMA are forthcoming. There's a potential need to standardize the CDA and CMA libraries on TypeScript and webpack for consistency, which I believe is crucial.

Change Summary

  • Upgraded to the latest versions of TypeScript and numerous other dependencies.
  • Incorporated the latest ESM-only dependencies, such as p-throttle.
  • Eliminated Rollup and bundle configurations, considering bundles are less ideal for node.js libraries intended as dependencies for other projects.
  • Transitioned from Jest to Vitest, allowing us to remove Babel and to finally get proper TS+ESM support in tests.
  • Discontinued the browser-specific build variant, acknowledging this package's primary function as a utility that does not run directly in browsers.
  • Simplified NPM scripts and streamlined package.json for better clarity.
  • The README has been updated to accurately reflect these modifications, along with previously unmentioned changes.
  • Cleaned up outdated elements, including remnants of chai, and removed workarounds for the prepublish script issue, which executed on every install.

Where we have to be careful

  • We should still ensure everything works well on all common environments
  • Our browser/node environment detection may not work anymore as expected. We could include testing the header into the test suite.

@axe312ger axe312ger requested a review from a team as a code owner February 19, 2024 16:19
@axe312ger axe312ger force-pushed the refactor/modern-esm-support branch 3 times, most recently from 883e962 to 21e0ac2 Compare February 22, 2024 14:31
@axe312ger axe312ger changed the title modern esm support WIP - modern esm support Feb 22, 2024
Copy link
Contributor

@mgoudy91 mgoudy91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code itself looks great to me! This is a significant improvement and jumps out to me as a methodical approach to making it happen.

A few questions I have about the outstanding work:

  1. This is a semantic version major/breaking release yes? Seems obvious but I'm not sure we've discussed it.
  2. What is your thought on the testing process for this? I'd love to be able to deploy a canary/beta version of this for testing (+the CMA/CDA js changes), and can help out with that if so, though I don't have much experience working in this repo generally. I can also help out with some manual testing as needed.

@axe312ger
Copy link
Contributor Author

The code itself looks great to me! This is a significant improvement and jumps out to me as a methodical approach to making it happen.

A few questions I have about the outstanding work:

1. This is a semantic version major/breaking release yes? Seems obvious but I'm not sure we've discussed it.

Yes, definitely a breaking change!

2. What is your thought on the testing process for this? I'd love to be able to deploy a canary/beta version of this for testing (+the CMA/CDA js changes), and can help out with that if so, though I don't have much experience working in this repo generally. I can also help out with some manual testing as needed.

I am testing currently extended on a local verdaccio server. As soon I am happy with it, I'll fix all the urls in the lock files to use public servers and we can deploy a public alpha version!

@axe312ger axe312ger changed the title WIP - modern esm support modern esm support Jun 19, 2024
@axe312ger axe312ger removed the WIP label Jun 19, 2024
@axe312ger
Copy link
Contributor Author

Will fix:

@axe312ger
Copy link
Contributor Author

This is published as 8.1.5-9.x-alpha.1 (Will be later published as v9 due to breaking changes)

Copy link
Contributor

@mgoudy91 mgoudy91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, missed a review here but I think this is still looking good since the last time

@axe312ger
Copy link
Contributor Author

@mgoudy91 isnt the contenful.js already released using this?

We probably should release this then as well.

@mgoudy91 mgoudy91 merged commit 0addd64 into master Sep 25, 2024
2 checks passed
@mgoudy91 mgoudy91 deleted the refactor/modern-esm-support branch September 25, 2024 16:35
@contentful-automation
Copy link

🎉 This PR is included in version 9.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants