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 React channel env missing in custom server #49168

Merged
merged 12 commits into from
May 3, 2023
Merged

Fix React channel env missing in custom server #49168

merged 12 commits into from
May 3, 2023

Conversation

shuding
Copy link
Member

@shuding shuding commented May 3, 2023

Fixes #48948 (repro). When running inside a custom server with app dir, we should always opt into the prebundled React with correct channels.

Thanks @karlhorky for help testing it!

Fixes #49169 too.

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels May 3, 2023
huozhi
huozhi previously approved these changes May 3, 2023
@ijjk
Copy link
Member

ijjk commented May 3, 2023

Failing test suites

Commit: 0bba771

pnpm testheadless test/e2e/app-dir/navigation/navigation.test.ts

  • app dir - navigation > redirect > components > should redirect to external url, initiating only once
Expand output

● app dir - navigation › redirect › components › should redirect to external url, initiating only once

expect(received).toEqual(expected) // deep equality

- Expected  - 1
+ Received  + 1

  Object {
-   "navigate-https://example.vercel.sh/": "1",
+   "navigate-https://example.vercel.sh/": "2",
    "navigation-supported": "true",
  }

  184 |           }
  185 |
> 186 |           expect(stored).toEqual({
      |                          ^
  187 |             'navigate-https://example.vercel.sh/': '1',
  188 |             'navigation-supported': 'true',
  189 |           })

  at Object.<anonymous> (e2e/app-dir/navigation/navigation.test.ts:186:26)

Read more about building and testing Next.js in contributing.md.

huozhi
huozhi previously approved these changes May 3, 2023
@sophiebits
Copy link
Contributor

(You wrote "Fixes #49168" but that's a link to this PR.)

@shuding
Copy link
Member Author

shuding commented May 3, 2023

Thanks @sophiebits, fixed. My clipboard is too slow.

ijjk
ijjk previously approved these changes May 3, 2023
@shuding
Copy link
Member Author

shuding commented May 3, 2023

The failing test is strange, can't reproduce it locally with both dev and prod. Will take a look later.

@shuding
Copy link
Member Author

shuding commented May 3, 2023

@sophiebits I saw your comments

// Not actually sure why this is '2' in dev. Possibly something
// related to an update triggered by <HotReload>?
'navigate-https://example.vercel.sh/': isNextDev ? '2' : '1',

and it's now always '1' with this change so tests are failing (I haven't looked into it), and hopefully that's not a bad thing? Can I just update the test?

This is interesting as I guess there might be some tests accidentally using the custom server mode and causes some inconsistency.

@shuding shuding dismissed stale reviews from ijjk and huozhi via 44415b7 May 3, 2023 21:45
@sophiebits
Copy link
Contributor

sure! seems fine

@shuding shuding merged commit 248f2de into canary May 3, 2023
@shuding shuding deleted the shu/9596 branch May 3, 2023 22:40
@shuding shuding mentioned this pull request May 3, 2023
kodiakhq bot pushed a commit that referenced this pull request May 3, 2023
Found this one being a bit random during dev, but it's trivial. See discussions: #49168 (comment)
@karlhorky
Copy link
Contributor

I can confirm next@13.3.5-canary.8 (first version including PR #49168) is working with custom servers 🎉 Thanks @shuding !!

CodeSandbox: https://codesandbox.io/p/sandbox/quirky-bell-o5tl8l?file=%2Fserver.mjs

Screenshot 2023-05-04 at 11 42 39

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
5 participants