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

Add js sourcemaps #122

Merged
merged 1 commit into from
Jun 29, 2018
Merged

Add js sourcemaps #122

merged 1 commit into from
Jun 29, 2018

Conversation

dmethvin-gov
Copy link
Contributor

Fixes #119

Additional config changes may be required in the sample app as well, so that the maps survive the long and treacherous journey through the final compressed JS files there. I'll check that out later.

Copy link
Contributor

@annekainicUSDS annekainicUSDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super familiar with source maps, so trying to learn about this still, but it seems a bit strange that it adds all of these files. When using sourcemaps with Webpack it doesn't seem to do that, so I'm not sure why that is yet. There seems to be an option to use inline source maps, which I think adds the path it directly in the original file, and it also is only used in development. Curious on your thoughts on this.

@dmethvin-gov
Copy link
Contributor Author

The browser is running a compiled version of the files that don't look like the source we edit. With a sourcemap, the browser can translate the location it's at in the JS or CSS file into the actual location in the original JSX or SCSS file that we edit, and show that instead.

There need to be separate map files because many of the the individual JS files are only included by an app if they need that functionality. So for example if an app doesn't use file fields on its form, it won't include fileField.js which is the compiled version of fileField.jsx. But if it does need file fields, it will include the JS file and dev tools will use fileField.js.map.

Inline maps can work okay for an app (vs a library) because the build step in the app decides whether the maps are needed or not (usually, development vs production). It's still very handy to have a map when debugging a production app so that you can tell what the original source looks like, so it's not necessarily true that you don't want a map for the production build. Sometimes that's when a map really saves your bacon, when you need to debug the live app.

However, it's even worse with a library. That's what #121 was about. The styles.css file went from about 150KB to 500KB when it used an inline map. Since you only need the map when you're actually inside dev tools and debugging, it's a big penalty to pay for the majority of the times when you have a debug build but aren't actually looking at the map. It also makes the library more error-prone to consume because the app that includes us-forms-system would need to strip out the inline maps somehow, or we'd have to publish two versions (production and debug), or we decide not to have maps at all. We publish both the JS and the JSX files so if we publish the map for each file as well it should be possible for an app writer to see the us-forms-system JSX which would make debugging easier.

Copy link
Contributor

@annekainicUSDS annekainicUSDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I wanted to test locally with us-forms-starter-app but was having difficulty because the commit isn't this repo. But I'm sure it'll be fine.

@dmethvin-gov dmethvin-gov merged commit 9638ac4 into usds:master Jun 29, 2018
@dmethvin-gov dmethvin-gov deleted the 119-addmaps branch June 29, 2018 16:30
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.

2 participants