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

fix(netlify): bundle netlify functions as ESM to support top-level await #8661

Merged
merged 3 commits into from
Sep 25, 2023

Conversation

Skn0tt
Copy link
Contributor

@Skn0tt Skn0tt commented Sep 25, 2023

Changes

#8598 included a change that makes Astro emit functions with top-level await, specifically this line:

const html = await updateImageReferences(${JSON.stringify(html)});

Top-Level await is supported in ESM, but not in CJS. By default, Netlify bundles .mjs files into CJS files under the hood, so this will result in build failures. To work around that, this PR adds a config file that tells Netlify to bundle into ESM.

Testing

I've tested this out on https://github.com/Hiroki1112/hiroki-dev, which is the site I first noticed this bug with. My change fixed their build process.

Docs

It's a fix, so no docs needed.

@changeset-bot
Copy link

changeset-bot bot commented Sep 25, 2023

🦋 Changeset detected

Latest commit: 76e5cb1

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Sep 25, 2023
@Skn0tt Skn0tt changed the title fix: bundle netlify functions as ESM to support top-level await fix(netlify): bundle netlify functions as ESM to support top-level await Sep 25, 2023
const functionsConfigPath = join(fileURLToPath(_config.build.server), "entry.json")
await writeFile(functionsConfigPath, JSON.stringify(functionsConfig))

// await writeFile(_config.build.server)
Copy link
Member

Choose a reason for hiding this comment

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

To remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, to remove! Not sure what happened there, should get some coffee :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 76e5cb1

Comment on lines +90 to +93
version: 1,
config: {
nodeModuleFormat: "esm"
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this documented somewhere? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, sadly not! It's more of a stop-gap solution until we start outputting to ESM for all functions - but there's always sites that break in that migration, so we're pretty careful about it. There's something in the works on this though, i'm hoping we can remove this again in a couple months! Will let you know about it then :)

@ematipico ematipico merged commit 008f764 into withastro:main Sep 25, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Sep 25, 2023
@Skn0tt
Copy link
Contributor Author

Skn0tt commented Sep 25, 2023

Thanks for the quick turnaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants