Skip to content

Conversation

@Brad-Turner
Copy link
Contributor

@Brad-Turner Brad-Turner commented May 24, 2025

This PR aims to improve ESM compatibility by removing the require call to load an external config file.

@Brad-Turner Brad-Turner marked this pull request as ready for review May 24, 2025 01:42
@Brad-Turner Brad-Turner requested a review from a team as a code owner May 24, 2025 01:42
@Brad-Turner
Copy link
Contributor Author

Hey @elylucas , any chance you can have a look over this PR please 🙏🏼

@Niko-Berry-Contentful
Copy link

Hey @Brad-Turner. Thank you so much for the PR. We're taking a look at this right now. To help us with the code review could you give us some more information about the context driving these changes?

  1. Could you elaborate on the specific ESM compatibility issue this PR resolves? Are there particular environments or bundlers where the current implementation fails?
  2. Is there a concrete use case or build system (e.g. Vite, Webpack, Node in ESM mode) where the existing require usage breaks execution or causes errors? If so, could you provide steps or a minimal reproduction?

@Brad-Turner
Copy link
Contributor Author

Hey @Niko-Berry-Contentful ,

Sorry for the delayed response. Busy time planning my wedding 😅

  1. Yep so this PR isn't going magically add full support for ESM, but it is a necessary task to help get this package there. In ESM, require syntax does not exist (this is a feature of commonjs). Dynamic imports are supported in both ESM and commonjs runtimes.
  2. This is only an error if you are running a node script (using ESM) and go to import and run this code programmatically. eg:
import contentfulExport from 'contentful-export';

await contentfulExport({ /* settings, etc */ }); 

At my work, we often script exports, transforms and uploads and this is kind of annoying when I have to switch between runtimes to export from/import to contentful.

@axe312ger
Copy link
Collaborator

axe312ger commented Jul 2, 2025

Hey @Brad-Turner,

thank you for this PR. You are right, in a pure ESM environment we won't have require available.

I have a few issue with your changes:

  1. a lot of formatting is touched, makes it hard to review and we may overlook a newly introduced bug/change there. Any chance you clean up the reformattings and only provide the actual changes to async/await + import()
  2. we will have to still support CJS environments, so have to be sure we provide this library in ESM + CJS
  3. We are working on ESM support for this and our other libs and it will come very soon. We should combine my changes there with yours!

Thank you so far,
Benedikt

Note to myself: We need to mirror this change to contentful-import as well

@axe312ger axe312ger self-assigned this Jul 2, 2025
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