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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn on sourcemaps in built dist #226

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

schneidmaster
Copy link

Hi there! Just a suggested improvement, feel free to ignore if you intentionally don't want sourcemaps for whatever reason 馃槃

I originally got here from this issue. Webpack's source-map-loader allows webpack to ingest sourcemaps from your dependencies and bundle them into the sourcemap for your application itself -- which is nice for debugging in general because if an error comes from within a library you can instantly trace it to the source instead of having to parse through minified output. However, with react-json-view installed, the source-map-loader logs an annoying warning.

Turns out the warning is because of this -- style-loader has a conditional to add a sourceMappingURL if the sourceMap configuration option is set. Note that the entire addStyles.js source is bundled into the dist when you're using style-loader (i.e. you can't just turn off the sourceMap config option -- it's actually off-by-default anyway -- because the whole conditional that checks whether it's on or off is inlined.) The source-map-loader picks up the sourceMappingUrl text out of this conditional and thinks it's an actual sourcemap comment, not code to maybe add a sourcemap comment.

The folks working on source-map-loader have seen this issue before -- here's an issue and here's the PR that came out of it. Their solution was to change source-map-loader to only parse the last sourceMappingURL found in the file (rather than all of them). This helps for most cases, but it doesn't actually help here because react-json-view doesn't have sourcemaps enabled in the built dist at all, so the last/only sourceMappingURL is the false positive from style-loader.

So, I propose that we turn on sourcemaps in the built dist. This is a double improvement for people who want to use source-map-loader -- they get sourcemaps out of react-json-view and they also get no annoying warning. It shouldn't really have much impact on anyone else since all it does is add a comment to the end of the built output (and include a sourcemap file adjacent to it in the dist directory).

I also removed the new webpack.optimize.UglifyJsPlugin() line because it's redundant (the --optimize-minimize in the package.json script adds it anyway) and was causing me some issues getting the build to work right with the sourcemaps.

Let me know what you think 馃槃

@josefernandez1
Copy link

Hi @schneidmaster , I'm having the same problem, as the PR is still open, did you find a work around to make the warning not appear?
Everything seems to work fine, but the error is there and it's a bit annoying.

@schneidmaster
Copy link
Author

@josefernandez1 Yes, to work around it I added an exclude line to my webpack config:

{
  test: /\.js$/,
  exclude: /react-json-view/,
  use: ['source-map-loader'],
  enforce: 'pre',
},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants