Skip to content
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

React component names mangling #4081

Closed
sergeyzwezdin opened this issue Feb 26, 2018 · 12 comments
Closed

React component names mangling #4081

sergeyzwezdin opened this issue Feb 26, 2018 · 12 comments

Comments

@sergeyzwezdin
Copy link

@sergeyzwezdin sergeyzwezdin commented Feb 26, 2018

Is this a bug report?

Yes. We aren't able to make automation testing.

Did you try recovering your dependencies?

Yes

Environment

  1. node -v: 9.4.0
  2. npm -v: 5.6.0
  3. yarn --version (if you use Yarn): 1.3.2
  4. npm ls react-scripts (if you haven’t ejected): react-scripts-ts@2.13.0

Then, specify:

  1. Operating system: Windows 10
  2. Browser and version (if relevant):

Steps to Reproduce

  1. create-react-app
  2. yarn build
  3. Open in browser → React DevTools

Expected Behavior

We would like to see unminified component's class names for our staging environment to make end-to-end automated testing. We use these names in our test cases, so we don't want to see them mangled.

Actual Behavior

Class names are mangled.

image

It's easy to fix by adding options to disable mangling in webpack.config.prod.js:

new webpack.optimize.UglifyJsPlugin({
	// ...
	mangle: {
		keep_classnames: true,
		keep_fnames: true
	},
	// ...
}),

Maybe adding the option to .env file is good way here?

@Timer

This comment has been minimized.

Copy link
Collaborator

@Timer Timer commented Feb 26, 2018

react-scripts-ts@2.13.0 you're on the wrong repo! 😄

@Timer Timer closed this Feb 26, 2018
@sergeyzwezdin

This comment has been minimized.

Copy link
Author

@sergeyzwezdin sergeyzwezdin commented Feb 27, 2018

I believe react-scripts-ts is in sync with react-scripts. Actually, if I create app based on react-scripts I'll have exactly same problem.

/cc @wmonk

@Timer

This comment has been minimized.

Copy link
Collaborator

@Timer Timer commented Feb 27, 2018

Oh, I'm sorry -- I misread this. I thought this was the development environment.

What you called "staging environment" is generated using our production build, which is always going to mangle class names in interest of file size.
Offering tunability in yarn build is a slippery slope, so I don't think we'll be exploring it.

@sergeyzwezdin

This comment has been minimized.

Copy link
Author

@sergeyzwezdin sergeyzwezdin commented Feb 28, 2018

Something which built by build server and pubished somewhere on a server. I don't ask to disable it, I would ask to give us an option to disable it.

@sergeyzwezdin

This comment has been minimized.

Copy link
Author

@sergeyzwezdin sergeyzwezdin commented Mar 1, 2018

@Timer @wmonk so, any update on this? I don't want to eject an app just because of one line in webpack config :-)

@wmonk

This comment has been minimized.

Copy link

@wmonk wmonk commented Mar 1, 2018

I don't believe this is a good option to add. Regarding you exact issue I suggest you use data tags to identify your components. Class names - even before minification like this existed - were inherently bad for identification due to the unfixed and subjective nature of naming. Using something like data-test-id="header-button" is preferable as it shows intent, and is unlikely to be refactored away.

@wmonk

This comment has been minimized.

Copy link

@wmonk wmonk commented Mar 1, 2018

Just reread your original post and not sure my comment solves your issue? What are you using the Component Class names for?

@sergeyzwezdin

This comment has been minimized.

Copy link
Author

@sergeyzwezdin sergeyzwezdin commented Mar 1, 2018

We have our own component hierarchy. The idea was to use component's class name to find it in tree. As for me data attributes looks like a hack.
Why do you think it isn't a good option to add? I believe each user of CRA should be able decide about resulting bundle shape.

@sergeyzwezdin

This comment has been minimized.

Copy link
Author

@sergeyzwezdin sergeyzwezdin commented Mar 16, 2018

@Timer @wmonk It seems the issue won't be fixed?

@Timer

This comment has been minimized.

Copy link
Collaborator

@Timer Timer commented Mar 26, 2018

No, sorry. Giving these sort of configuration options are outside of our values.
If you feel strongly about this, you'll need to eject.

@idolizesc

This comment has been minimized.

Copy link

@idolizesc idolizesc commented Oct 4, 2018

One use case where this kind of functionality would be helpful in production is for componentStack error handling in componentDidCatch when sourcemaps are not available client side (e.g. when using Sentry for server side sourcemapping).

As it is, the componentStack is unusable in such cases. @Timer

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Oct 4, 2018

One thing you can do is to explicitly give some components displayName. Minifier won't remove it.

function Stuff() {}
Stuff.displayName = 'Stuff'

Giving it to every component is overkill and will bloat your bundle. But if you strategically pick a few dozen components (depending on your product size) I think it will give you a good balance.

@lock lock bot locked and limited conversation to collaborators Jan 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.