-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
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
[utils] Fix isPlainObject to work across realm #39981
Conversation
Netlify deploy previewhttps://deploy-preview-39981--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
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.
@brijeshb42 I need more context. Is this the only function that does not work with edge runtime? How do I test locally that the change is not working?
@siriwatknp You can use a local next.js project with edge runtime in dev mode. I followed the instructions mentioned #39338. This is the repo. This was resulting in critical tokens being undefined and throwing errors. |
0dd31e9
to
4b68ad9
Compare
Does it relate to #23739 (comment)? |
Yes. Seems root cause is same for both the issues. Edit: The core issue is similar but in this particular, it's not about checking objects across realms. Instead, here, the realm itself is different. |
Ok, then I think to bring the regression test in, and to consider if the implementation can't be simplified to improve performance. I suspect lodash tests for cases we don't need to support. |
4b68ad9
to
490491e
Compare
490491e
to
92ece07
Compare
When will this pull request be merged into the master branch? |
92ece07
to
e5f8e17
Compare
This update adds support for checking plain object in non-v8/standard runtimes as well (ie vercel edge-runtime)
e5f8e17
to
b891ccf
Compare
I rebased on HEAD. |
deepmerge({}, JSON.parse('{ "myProperty": "a", "__proto__" : { "isAdmin" : true } }'), { | ||
clone: false, | ||
}); | ||
const result = deepmerge( | ||
{}, | ||
JSON.parse('{ "myProperty": "a", "__proto__" : { "isAdmin" : true } }'), | ||
{ | ||
clone: false, | ||
}, | ||
); | ||
|
||
expect(result).not.to.have.property('isAdmin'); |
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.
This change is missing the point of the test. Reverted in fd12e42.
I pushed a new commit to remove vm-browserify. It feels like we don't need to run the test in a new iframe, that the node vm module is enough, keeping the stack simpler. |
This update adds support for checking plain object in other runtimes as well (ie vercel edge-runtime)
The implementation has been borrowed from https://github.com/sindresorhus/is-plain-obj to support our bare-minimum use-case.
Other simpler way would be to also add check for edge runtime but it will only cover Vercel's runtime and will be a short-term fix.
Closes #23739
Closes #39338
Closes #38177