-
Notifications
You must be signed in to change notification settings - Fork 26.7k
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 build worker callback arg missing correct page path #61347
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Stats from current PRDefault BuildGeneral
Client Bundles (main, webpack)
Legacy Client Bundles (polyfills)
Client Pages
Client Build Manifests
Rendered Page Sizes
Edge SSR bundle Size
Middleware size
Next Runtimes
|
huozhi
requested review from
timneutkens,
ijjk,
shuding,
feedthejim,
ztanner,
wyattjoh and
a team
as code owners
January 29, 2024 19:51
ijjk
approved these changes
Jan 30, 2024
styfle
reviewed
Jan 30, 2024
styfle
approved these changes
Jan 31, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, we can improve the types in a follow up PR 👍
I just want to make sure the types are correct so the next person doesn't create a new bug 😅
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
The
arg
in the worker callback is alwasyany
, when we access the page path the argument could be in different form as the arg types are different.This PR align the argument type to object for
isPageStatic
,getDefinedNamedExports
,hasCustomGetInitialProps
methods in static worker. So they can share the similar shape of type of argument when accessingpage
path. This will avoid the case that loggedpage
in the warning isundefined
Import the helpers from utils instead of workers as the worker itself don't need to contain other exports that is not used for static worker.
Why
This PR align the callback type of callback argument type of static worker, so that we could get the actual page path value in a type-safe way. We have 4 methods for static worker,
exportPage
,isPageStatic
,getDefinedNamedExports
,hasCustomGetInitialProps
, which the rest of 3 methods share the same format of warnings but their argument are different. It's easily ended up with wrong argument type, and log with a bad page path in the warning.Closes NEXT-2289