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

fix: store fetchEvent in h3 context #1586

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

katywings
Copy link
Contributor

@katywings katywings commented Jul 21, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • infrastructure changes
  • Other... Please describe:

What is the current behavior?

getFetchEvent is flaky, creating & storing multiple fetchEvents per h3Event. The reason for this is vinxi duplicating the code including the fetchEventSymbol per router (nksaraf/vinxi#51), e.g. if you console.log the symbol

const fetchEventSymbol = Symbol("fetchEvent");
you will notice that the console.log is executed twice during startup. Also if you log Object.getOwnPropertySymbols(h3Event) in getFetchEvent you get [ Symbol(fetchEvent), Symbol(fetchEvent) ] on requests that affect multiple vinxi routers.

What is the new behavior?

getFetchEvent will maximally create one fetchEvent per h3Event by storing the fetchEvent in h3Event.context. h3Event is stable across multiple routers and h3Event.context is specifically made for situations like this as @pi0 already confirmed in an other thread (#1203 (comment)).

Copy link

changeset-bot bot commented Jul 21, 2024

🦋 Changeset detected

Latest commit: 8515f2b

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

This PR includes changesets to release 1 package
Name Type
@solidjs/start 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

@katywings katywings force-pushed the fix-flaky-request branch from cf5f476 to 8515f2b Compare July 21, 2024 11:43
@boehs
Copy link
Contributor

boehs commented Jul 21, 2024

lgtm, but why are symbols no longer being used? I assume the context supports them?

@katywings
Copy link
Contributor Author

@boehs You can think of the symbol like an object, when you create one its always a new one. And there exist multiple ones because vinxi duplicates the code.

Maybe this helps to explain:

Symbol("foo") === Symbol("foo"); // false
"foo" === "foo" // true

@ryansolid
Copy link
Member

Interesting. Well good catch.

@ryansolid ryansolid merged commit 65c8ac0 into solidjs:main Jul 23, 2024
3 checks passed
# 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