Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

make sure edge function entries are properly awaited #704

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

dario-piotrowicz
Copy link
Contributor

the values in self._ENTRIES can be promises so they need to be awaited before using them, AFAICT that's also what Next.js does: source code

this PR makes sure that such values are properly awaited

Copy link

changeset-bot bot commented Jan 20, 2025

🦋 Changeset detected

Latest commit: e4c87e3

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

This PR includes changesets to release 3 packages
Name Type
@opennextjs/aws Patch
app-pages-router Patch
app-router Patch

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

Copy link

pkg-pr-new bot commented Jan 20, 2025

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/aws@704

commit: e4c87e3

@dario-piotrowicz dario-piotrowicz force-pushed the dario/edge-function-async-entry branch from ddc1848 to 6d87f6f Compare January 20, 2025 13:03
Copy link
Contributor

github-actions bot commented Jan 20, 2025

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 23.46% 1878 / 8005
🔵 Statements 23.46% 1878 / 8005
🔵 Functions 56.41% 110 / 195
🔵 Branches 70.72% 471 / 666
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/open-next/src/core/edgeFunctionHandler.ts 0% 100% 100% 0% 7-34
packages/open-next/src/types/global.ts 0% 0% 0% 0%
Generated in workflow #897 for commit e4c87e3 by the Vitest Coverage Report Action

Copy link
Contributor

@conico974 conico974 left a comment

Choose a reason for hiding this comment

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

LGTM Thanks.
Could you just add a changeset though ?

Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

LGTM.

Is there a simple way to add a test?

@dario-piotrowicz
Copy link
Contributor Author

LGTM Thanks. Could you just add a changeset though ?

Thanks 🙂, sorry my bad, here I've added one: e4c87e3 let me know if it's ok or if it should contain more details 🙂

@dario-piotrowicz
Copy link
Contributor Author

Is there a simple way to add a test?

good point, let me have a look 👍

@dario-piotrowicz
Copy link
Contributor Author

LGTM.

Is there a simple way to add a test?

Since this relates to the code we get from Next.js itself I don't think it makes sense to unit test it, an e2e would make sense to me here, however the e2e tests setup here is pretty involved and it'd take me a while to get it done 😓

@conico974 has been kind enough to offering to add an e2e test for this in a followup PR (thanks a lot @conico974! ❤️ )

so if everyone is ok with it I'd say to just merge the PR as is 😄

@vicb vicb merged commit e5678b3 into main Jan 20, 2025
3 checks passed
@vicb vicb deleted the dario/edge-function-async-entry branch January 20, 2025 14:27
@github-actions github-actions bot mentioned this pull request Jan 20, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants