facebook / create-react-app

Set up a modern web app by running one command.
https://create-react-app.dev
MIT License
102.76k stars 26.87k forks source link

Question: Source mapping componentStack from error boundary in production #6667

Open danielkcz opened 5 years ago

danielkcz commented 5 years ago

Lately, I am trying to use error boundaries to tackle unexpected errors more properly. I am also logging most of the errors to the Sentry service. I am sending the componentStack from componentDidMount there as well, but since it's minified, it feels fairly useless.

image

I am wondering what are my options if any. Since that output is pretty much just a string, the source mapping does not seem possible at any level.

iansu commented 5 years ago

You can upload your sourcemaps to Sentry. That might help in this situation.

danielkcz commented 5 years ago

@iansu That surely won't work because the componentStack is just a string, there is no structured information to feed source mapping mechanism. Have a look at this example (used to showcase different issue).

https://codesandbox.io/s/zw500lpm2p

atomicleads commented 5 years ago

Source map upload definitely doesn't help. I have it uploaded, my stack trace is decoded properly, but componentStack is not.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

danielkcz commented 5 years ago

Oh come on, I hate this little blue thing. Just because there is no viable answer yet, it shouldn't be stale :( This is a real issue. Can someone please tell that robot it's ok to keep it open?

jeremygottfried commented 5 years ago

Hi @iansu are the any updates on this issue? This is a significant for those of us who want to use error boundary to log errors.

iansu commented 5 years ago

There is no update on this. As far as I know no one is working on it.

Tonyce commented 5 years ago

I have the same problem。Since React have given this component idea,I also expect a way.

robertvansteen commented 5 years ago

Duplicate of https://github.com/facebook/create-react-app/issues/3753

danielkcz commented 5 years ago

@rovansteen You are right, it feels like duplicate, but I think it's still worth revisiting. In that linked issue there is no mention why that stack does not include line numbers. Something similar to a development version. It's totally fine if component names are minified, but if we had some location in a compiled bundle, it's another piece of information to ease the pain of finding the problem.

robertvansteen commented 5 years ago

I agree, but as far as I understand that is something for https://github.com/facebook/react. Not much what we can do here.

danielkcz commented 5 years ago

Well, if I understand it correctly, for development it's done by transform-react-jsx-source Babel plugin. That would mean it's more about CRA config than React.

I did found https://reactjs.org/docs/error-boundaries.html#component-stack-traces which mentions the plugin shouldn't be used in production, but not sure why really. Does it lower performance significantly? I haven't found any information regarding that.

Tonyce commented 5 years ago

Well, I have got my solution. Could get the stack str from error. Thanks

danielkcz commented 5 years ago

@Tonyce That's really nice of you that you want to share your solution ...

sergei-startsev commented 5 years ago

@FredyC In the same doc (https://reactjs.org/docs/error-boundaries.html#component-stack-traces) they also mentioned displayName property that can be used to get a proper component name displayed in the stack traces.

There's a babel plugin that allows to get a name with relative file path prefix in production.

danielkcz commented 5 years ago

@sergei-startsev Ok that plugin is interesting.

@rovansteen Is it possibly something to consider to include in CRA?

robertvansteen commented 5 years ago

I don't think we want to include something like this by default as it will increase the bundle size for everyone. So if something like this will be included it needs to be opt-in but I don't know if we want to go down that road of making the babel setup configurable.

I'll open this again as a proposal and see what other maintainers think of this.

Tonyce commented 5 years ago

@FredyC

  ...
       const stackArr = body.split('\n');
        console.log(stackArr);
        const fileInfo = stackArr[1].split('/').pop();
        const [file, line, column] = fileInfo.split(':');
        console.log(file, line, column);
        const fileMap = await fsReadFile(`maps/${file}.map`, { encoding: 'utf8' });
        // console.log(fileMap);
        const consumer = await new sourceMap.SourceMapConsumer(fileMap);
        // doStuffWith(consumer);
        const result = consumer.originalPositionFor({ line: Number(line), column: Number(column.replace(')', '')) })
        console.log(result);
...
  xhr.send(e.stack); // e is the error
...

Is this demo clear ?

danielkcz commented 5 years ago

@Tonyce Thanks for sharing. So to have a correct error stack in production you are including extra 28kB in the bundle? Not exactly a slim solution. Also not sure what that fsReadFile is and how big it is.

Besides, it seems this will only map toward error stack trace that's mostly in React code, but not the actual component source files like seen in the other issue.

Nah, unless I am misunderstanding something here, then the above-mentioned plugin with auto attaching displayName still feels like a better solution given it only adds a couple of extra strings to bundle instead of the whole bunch of runtime code to tackle this.

Tonyce commented 5 years ago

@FredyC fsReadFile just is fs.readFile in nodejs. in prod, i am not serve the mapfile, just keep them in the server side.

danielkcz commented 5 years ago

@Tonyce Oh, that's server-side, that makes more sense now. We are talking here about client-side caching of errors so please next time read carefully before you start announcing you got a solution :)

rmehlinger commented 5 years ago

To fix this, consider switching to Terser as your minification plugin. Terser provides an option to not mangle function names, which includes arrow functions, and also to not mangle class names: keep_fnames and keep_classnames. Both of these options take regexes. Since React component names are capitalized, you can pass the regex /[A-Z].+/and that should only hit your components (especially if you enforce camel casing for other functions with a linter). You'll get a slightly larger bundle size, but unless you're working on something that needs to be hyper-optimized it should be within tolerances.

Sample webpack config:

      new TerserJsPlugin({
        terserOptions: {
          output: {
            comments: false,
          },
          keep_classnames: true, // any classes are probably React Components anyway
          keep_fnames: /[A-Z].+/,
        },
        sourceMap: true,
      }),
danielkcz commented 5 years ago

@rmehlinger That's not an exactly flexible solution. Assuming all classes and functions are components is rather shallow thinking :)

Generally, people are rather sensitive to bundle size, so making it bigger to allow slightly better debugging is not going to pass through here.

Besides, we already have a suggestion for a smaller solution that attaches displayName to components only, so other things stay mangled as they should. But even that might not be justifiable to be enabled for everyone.

rmehlinger commented 5 years ago

Capitalized functions likely are components, though, especially as one can use a linter to enforce that other function names are lower-cased. Anyway... it's an option.

EDIT: Also, upon trying react-displayname-path, it appears to not work out of the box on functional components, though it does work on my class components. I suspect this has something to do with writing in TypeScript, but I'm not sure.

jeremygottfried commented 5 years ago

I like the idea of setting displayName property. For most of my cases, knowing the name of the main container component will suffice, so I think it's a bit overkill to set displayName of every component.

osdiab commented 4 years ago

Any updates on this issue? I'm interested in this as well.

freemaths commented 3 years ago

Also interested. In particular I log error stack traces from live but I need to map back to non-minimised line/column (I have the generated source maps). I have tried using SourceMapConsumer from the https://github.com/mozilla/source-map package but it requires a .wasm file. Can create-react-app generate a wasm file or is there an alternative way to do the equivalent of: consumer.originalPositionFor({ line: line, column: column})

lijunle commented 3 years ago

I have an idea. Instead of printing the little t in the component stack, could we print it with where the class is defined - the full JavaScript file path + line/column number.

That does not introduce any overhead in user React component code, maybe a little more code in React library code. It increases the weights of requests to logging system. But I think that should be fine - the weight here is helping debugging.


Update

Did more research and found React 17 has already this feature!

https://codepen.io/lijunle/pen/gOWrZdR?editors=0011

image

bvaughn commented 3 years ago

Also interested. In particular I log error stack traces from live but I need to map back to non-minimised line/column (I have the generated source maps). I have tried using SourceMapConsumer from the mozilla/source-map package but it requires a .wasm file. Can create-react-app generate a wasm file or is there an alternative way to do the equivalent of: consumer.originalPositionFor({ line: line, column: column})

@freemaths No opinion on whether or not this is a good idea, but you should be able to copy the wasm file from node_modules if you really want to do this:

cp ./node_modules/source-map/lib/mappings.wasm ./public/mappings.wasm

Then you can initialize it like:

SourceMapConsumer.initialize({'lib/mappings.wasm': '/mappings.wasm'});