Closed unekinn closed 3 years ago
bvaughn
commented
3 years ago I see a slightly different error than the one reported:
Uncaught TypeError: Result of the Symbol.iterator method is not an object
at Function.from (<anonymous>)
at formatDataForPreview (react_devtools_backend.js:849)
at dehydrate (react_devtools_backend.js:1718)
at react_devtools_backend.js:1769
...I can repro this though so that's good. I can fix this.
bvaughn
commented
3 years ago Fix will go out with the next release.
unekinn
commented
3 years ago Not sure what your release cycle is, but I'm using the extension built from source in the meantime. Works fine now, thanks!
ivanhofer
commented
3 years ago I have a similar issue. I created a PR that should fix the crash: https://github.com/facebook/react/pull/22380
To reproduce:
cd into examples/reactnpm inpm run devComponents-Tab in React Dev ToolsChild componentyou should see following error:
bvaughn
commented
3 years ago @ivanhofer I would appreciate if you could make a reduced repro that didn't import other frameworks (like this i18n framework). Seems like the core of this problem is Proxy related, but I'm having to dig through unfamiliar (TypeScript) code trying to narrow down why it's still throwing in #22380.
ivanhofer
commented
3 years ago @bvaughn thanks for your help!
I simplified the example a bit and marked the key-part. Just replace the file src/i18n/i18n-react.tsx with following content:
import React from 'react'
export const getFallbackProxy = (prefixKey?: string): any =>
new Proxy((prefixKey ? () => prefixKey : {}) as any, {
get: (_target, key: string) => getFallbackProxy(prefixKey ? `${prefixKey}.${key}` : key),
})
export const initI18nReact = () => {
const context = React.createContext({} as any)
const Component: React.FunctionComponent<{ initialLocale: string }> = (props) => {
const [isLoadingLocale, setIsLoadingLocale] = React.useState<boolean>(false)
const [currentLocale, setCurrentLocale] = React.useState<string | null>(null)
const [LL, setLL] = React.useState(getFallbackProxy())
const setLocale = async (newLocale: string): Promise<void> => {
setIsLoadingLocale(true)
setLL(
// ------------------------------------------------------------------------------------
// KEY-PART BEGIN ---------------------------------------------------------------------
// ------------------------------------------------------------------------------------
new Proxy(
{},
{
get: (target: any, key: string) => {
console.log(`accessed:`, key)
return () => key
},
},
) as any,
// ------------------------------------------------------------------------------------
// KEY-PART END ---------------------------------------------------------------------
// ------------------------------------------------------------------------------------
)
setCurrentLocale(newLocale)
setIsLoadingLocale(false)
}
!currentLocale && !isLoadingLocale && setLocale((props as any).initialLocale || 'en')
if (!isLoadingLocale && !LL) {
return null
}
const ctx = { setLocale, isLoadingLocale, locale: currentLocale, LL }
return <context.Provider value={ctx}>{props.children}</context.Provider>
}
return { component: Component, context }
}
// --------------------------------------------------------------------------------------------------------------------
const { component: TypesafeI18n, context: I18nContext } = initI18nReact()
export { I18nContext }
export default TypesafeI18nand then run npm run start
When you look at the console output you can see all access-attempts to the proxy being logged:
The uppercased names come from the application-code itself. Then it gets interesting.. React Dev Tools tries to access that proxy with a lot of different keys, the Proxy can not handle (and should not handle). Not so sure where this gets called and I currently don't have a lot time to dig deeper..
I assume this PR is 'fixing' the wrong thing since the problem lays somewhere else.
bvaughn
commented
3 years ago I'm seeing a different error from the repro you shared above:
Uncaught TypeError: Result of the Symbol.iterator method is not an object
The error I was seeing on the PR was:
Uncaught DOMException: Failed to execute 'postMessage' on 'Window': function () { [native code] } could not be cloned.
ivanhofer
commented
3 years ago But you see all the accesses to the Proxy from React Dev Tools? Forget the error for now. If we can tell React Dev Tools to not access the Proxy, the error should be gone.
bvaughn
commented
3 years ago But you see all the accesses to the Proxy from React Dev Tools? Forget the error for now. If we can tell React Dev Tools to not access the Proxy, the error should be gone.
It's not as simple as that. 😄
When an element is inspected, DevTools must serialize its props/state/hooks (using postMessage) to send values from the "backend" (running in the page with your app) to the "frontend" (DevTools extension UI). (These are in separate memory spaces, so we can't just share the value directly.)
As a side note, it also needs to avoid deeply cloning objects because that can add a lot of overhead and slow things down, so instead it shallowly clones them before serializing.
The main utility that does this is here: https://github.com/facebook/react/blob/033efe7312cdf73118922b279d9b1ae29a2f693d/packages/react-devtools-shared/src/hydration.js#L102-L335
The code path that's triggering all of the accesses you're pointing out is here: https://github.com/facebook/react/blob/033efe7312cdf73118922b279d9b1ae29a2f693d/packages/react-devtools-shared/src/hydration.js#L302-L320
Which calls this function to loop over props: https://github.com/facebook/react/blob/033efe7312cdf73118922b279d9b1ae29a2f693d/packages/react-devtools-shared/src/utils.js#L77-L97
Then eventually this method throws when trying to stringify a property: https://github.com/facebook/react/blob/033efe7312cdf73118922b279d9b1ae29a2f693d/packages/react-devtools-shared/src/utils.js#L685-L863
More specifically, this line: https://github.com/facebook/react/blob/033efe7312cdf73118922b279d9b1ae29a2f693d/packages/react-devtools-shared/src/utils.js#L784-L788
Calling Array.from with the proxy value throws. Replacing that method of mapping keys with a for...of that pushes to an array also throws. You can reproduce this error with the following stripped down code:
const getFallbackProxy = (prefixKey) =>
new Proxy(prefixKey ? () => prefixKey : {}, {
get: (target, key) =>
getFallbackProxy(prefixKey ? `${prefixKey}.${key}` : key),
});
const proxy = getFallbackProxy();
for (const key of proxy) {
// ...
}Q: Why can't you just treat this Proxy object differently?
A: Because (to my knowledge) there is no way to determine if a value is a Proxy.
Q: Is there a workaround for this?
A: You could make your Proxy detectable using the instanceof operator, but it would require custom work on your side to support. You could also defined a custom ownKeys accessor to limit the values that were iterated over.
Q: Is there something (hacky) that DevTools could do to detect this case.
A: Maybe? Maybe we could check something like data.__proto__ !== data.__proto__ to detect an edge case like this, but I'm not sure if this would have other negative consequences. I'm also not sure what we would do with it once we had detected it. Just show a special shallow "[[Proxy]]" string or something? It wouldn't be inspectable.
ivanhofer
commented
3 years ago @bvaughn thanks for your detailed analysis.
In my opinion the best solution would be to define the ownKeys method on the proxy-handler.
I tested it with the current React Dev Tools but it seems you don't jet check the available properties with Object.getOwnPropertyNames() am I right?
So work on both ends would be required.
bvaughn
commented
3 years ago In my opinion the best solution would be to define the
ownKeysmethod on the proxy-handler.
This sounds like a reasonable approach.
I tested it with the current React Dev Tools but it seems you don't jet check the available properties with
Object.getOwnPropertyNames()am I right?
That's right. I don't think it's generally desirable that we limit the values displayed to being only properties found directly on the object. That being said, maybe we could do something like this as a fallback.
let array;
try {
array = Array.from(data);
} catch (error) {
if (
error.message === "Result of the Symbol.iterator method is not an object"
) {
// The value in question may be a Proxy object,
// in which case the best we can do here is fall back to using its 'ownKeys' value.
array = Object.keys(data).map(value => [value]);
} else {
throw error;
}
}However, even after doing something like this, I'm seeing a different runtime error.
I would welcome proposals/contributions from you to improve the way DevTools works with edge case objects like the type of Proxy this library uses.
ivanhofer
commented
3 years ago @bvaughn not so sure if detecting a proxy by the content of the error-message is a good solution.
Is there another way to check if we access an object? Maybe use something like:
let array
if (data instanceof Array) {
array = Array.from(data)
} else {
array = Object.keys(data)
}I don't think that approach makes sense. If it's already an array, we wouldn't need to convert it. The purpose of calling Array.from is to simplify handling different types of iterables.
That being said, I think even if we solve the iteration issue (which the error message check does, even if not in a very robust way) I think we'll still have other issues.
ivanhofer
commented
3 years ago Ah, sorry, I didn't check the context of that array example. This code is here to display a graphical representation of an object somewhere inside the React Dev Tools?
I think a better solution would be to change this https://github.com/facebook/react/blob/033efe7312cdf73118922b279d9b1ae29a2f693d/packages/react-devtools-shared/src/utils.js#L597 to
try {
const iterator = data[Symbol.iterator]();
if (!iterator) {
// Proxies might break assumptions about iterators.
// See github.com/facebook/react/issues/21654
} else {
return iterator === data ? 'opaque_iterator' : 'iterator';
}
} catch (e) {
return 'proxy'
}and add a new 'proxy' case inside the formatDataForPreview function that uses Object.keys(data)
bvaughn
commented
3 years ago @ivanhofer I don't think the above code works, because it would hit false positives for HTML collectins or regular expressions. The only way I know of to check this would be to use the non-standard __proto__ property (which seems like not a great idea).
ivanhofer
commented
3 years ago @bvaughn in other words: we are not able to find a solution to the problem? So also HTML collections and regular expressions throw an error in the same line as a Proxy?
But looking at the getDataType function you have already a way to detect these two types. So I don't see why the code mentioned above should not work.
It could indeed create false positives, but at least it will not throw an error.
bvaughn
commented
3 years ago we are not able to find a solution to the problem?
That is correct.
So also HTML collections and regular expressions throw an error in the same line as a Proxy?
They would if you tried to invoke data[Symbol.iterator]().
Look at the surrounding code: https://github.com/facebook/react/blob/f2c381131fb58c107e6153255bc8d1d6340db506/packages/react-devtools-shared/src/utils.js#L596-L613
We only enter the Symbol.iterator code path for "iterator" (and "opaque_iterator"). Your proposal, seemingly, was to enter it always and interpret a catch as a Proxy detection. This would break the other types I mentioned.
Alternately, if we left this check in place, then the try/catch would never run for Proxies:
https://github.com/facebook/react/blob/f2c381131fb58c107e6153255bc8d1d6340db506/packages/react-devtools-shared/src/utils.js#L596
ivanhofer
commented
3 years ago @bvaughn I just created a PR that shows the implementation I would imagine.
It has no side-effect on the detection of regular expressions and collections.
It just handles cases, where accessing and executing e.g. the 'constructor' property of a proxy throws an error.
bvaughn
commented
3 years ago I tried an approach very similar and it didn't work for me (in Node) until I removed the typeof check above. That's interesting. can you add a failing test to Inspectedelement-test
ivanhofer
commented
3 years ago yes, just give me a few days to find time for that
bvaughn
commented
3 years ago Sounds good!
Website or app
https://codesandbox.io/s/react-devtools-crash-with-proxy-in-hook-dxd32?file=/src/App.tsx
Repro steps
Provided is a code sandbox with a very simplified version of the code which triggered this bug.
Observe that devtools crashed, and the console prints out an error similar to this:
This happens in both Firefox and Chrome.
How often does this bug happen?
Every time
DevTools package (automated)
react-devtools-extensions
DevTools version (automated)
4.13.5-0ae5290b54
Error message (automated)
Could not inspect element with id 2
Error call stack (automated)
No response
Error component stack (automated)
GitHub query string (automated)