Fix lazy not to throw undefined objects #18928
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 3beb303:
|
|
Comparing: 9d17b56...849c264 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
b4h0-c4t
force-pushed
the
fix-lazy-throw-error-object
branch
2 times, most recently
from
53110d8 to
92505e9
Compare
b4h0-c4t
force-pushed
the
fix-lazy-throw-error-object
branch
from
92505e9 to
3beb303
Compare
| moduleObject => { | ||
| if (payload._status === Pending) { | ||
| const defaultExport = moduleObject.default; | ||
| const defaultExport = moduleObject && moduleObject.default; |
There was a problem hiding this comment.
Similarly, why isn't this property access throwing a proper error that then is passed to the error handler?
There was a problem hiding this comment.
Should I use 'invariant' for the check?
There was a problem hiding this comment.
If throwing error (invariant or other) in thenable.then, it error to be UnhandledPromiseRejectionWarning.
I think that it is a better solution than before the fix.
|
@sebmarkbage 1. Allowing the moduleObject to be undefined. 2. Use invariant. moduleObject => {
if (payload._status === Pending) {
invariant(
moduleObject !== undefined,
'lazy: lazy: Expected the result of a dynamic import() call. '
);
const defaultExport = moduleObject.default;
// ... some pragram.It can throw an error using invariant. 3. Use Rejected moduleObject => {
if (payload._status === Pending) {
// Transition to the next state.
if(moduleObject !== undefined){
const defaultExport = moduleObject.default;
const resolved: ResolvedPayload<T> = (payload: any);
resolved._status = Resolved;
resolved._result = defaultExport;
} else {
const rejected: RejectedPayload = (payload: any);
rejected._status = Rejected;
rejected._result = error;
}
// ... some pragram.It takes the "rejected status" to onFulFill. |
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
|
bump |
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
|
bump |
|
What is the status of this PR? throwing undefined is still happening |
|
I think my turn to be over, but what do the maintainers think? |
| "366": "ReactDOM.createEventHandle: setListener called on an target that did not have a corresponding root. This is likely a bug in React.", | ||
| "367": "ReactDOM.createEventHandle: setListener called on an element target that is not managed by React. Ensure React rendered the DOM element.", | ||
| "368": "ReactDOM.createEventHandle: setListener called on an invalid target. Provide a valid EventTarget or an element managed by React.", | ||
| "368": "ReactDOM.createEventHandle: setListener called on an invalid target. Provide a vaid EventTarget or an element managed by React.", |
There was a problem hiding this comment.
Not reviewing this PR, just glancing over it and noticed this change that looks to have been an accident and should be reverted.
|
I think there is a misunderstanding here. The original review asked: why is it throwing undefined instead of the original error propagating? This question has not been answered as far as I can see. This is what you want to answer before making any code changes. |
|
I sent #21639 with potential fix. Not sure if it works yet. |
|
We ended up doing this differently (#21642), but thank you for nudging to get this done. I agree it was an annoying oversight. |
8 participants
Summary
fixed #18768
This issue has two problems.
First problem
React.lazydid not perform an undefined check formoduleObject.input
current
fixed
Second problem
React.lazydidn't check if the result of executing the argument was thenable.input
current
fixed
Test Plan
yarn test ReactLazy