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

enable local css source maps with flag #9415

Merged
merged 6 commits into from
Jan 21, 2019

Conversation

rianfowler
Copy link

@rianfowler rianfowler commented Jan 18, 2019

Description

This is an attempt to prevent the npm watch task from crashing after multiple re-builds are triggered. The issue was tracked down to css-loader having source maps on by default. Memory profiles suggest that webpack or the css-loader is retaining old build's strings in memory.

I believe this is the core issue and will revisit adding css source maps to the default watch task when it's resolved.

This change turns off css sourcemaps by default for local development. A new script has been added that

  • turns on css source maps
  • increases the memory node is allowed to use

The new script will probably still crash after node runs out of memory but this will give it more room to fail and limit that failure to work that requires debugging css with sourcemaps.

Testing done

npm run watch:css-sourcemaps after 20+ rebuilds:
screen shot 2019-01-18 at 13 31 46

npm run watch after 20+ rebuilds:
screen shot 2019-01-18 at 13 35 49

Screenshots

Acceptance criteria

  • [ ]

Definition of done

  • Events are logged appropriately
  • Documentation has been updated, if applicable
  • A link has been provided to the originating GitHub issue (or connected to it via ZenHub)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs

@va-bot va-bot temporarily deployed to vetsgov-pr-9415 January 18, 2019 20:45 Inactive
Copy link
Contributor

@jbalboni jbalboni left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me, curious to see what @ncksllvn thinks.

@va-bot va-bot temporarily deployed to vetsgov-pr-9415 January 18, 2019 20:59 Inactive
@va-bot va-bot temporarily deployed to vetsgov-pr-9415 January 18, 2019 21:00 Inactive
Copy link
Contributor

@ncksllvn ncksllvn left a comment

Choose a reason for hiding this comment

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

LGTM, although I wonder if there's a setting or different type of source map that could help free up some memory. It might be worth experimenting with this line sometime and see if it makes any difference.

Sometime, I wonder if we could implement a help command or something as a way of documenting these args. This seems like one that feels natural as just as an extra arg passed to the regular watch task instead of a separate way of starting the site, but without it in the package.json it's hard to make it visible.

@@ -43,6 +43,10 @@ const configGenerator = (buildOptions, apps) => {
ENVIRONMENTS.VAGOVPROD,
].includes(buildOptions.buildtype);

const enableCSSSourcemaps =
buildOptions.buildtype !== ENVIRONMENTS.LOCALHOST ||
buildOptions['local-css-sourcemaps'];
Copy link
Contributor

Choose a reason for hiding this comment

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

If buildOptions['entry'] is passed, should it flip to true? Just thinking that if we've limited what apps we're compiling, then maybe it should consider CSS sourcemaps to be safe too.

@@ -24,7 +24,8 @@ very secret.
| build the production site (dev features disabled). | `npm run build -- --buildtype vagovprod` Note the extra `--` is required otherwise npm eats the buildtype argument instead of passing it on. |
| build the site with optimizitons (minification, chunking etc) on. | Set `NODE_ENV=production` before running build. |
| reset local environment (clean out node modules and runs npm install) | `npm run reset:env` |
| run the site for local development with automatic rebuilding of Javascript and sass | `npm run watch` then visit `http://localhost:3001/`. You may also set `buildtype` and `NODE_ENV` though setting `NODE_ENV` to production will make incremental builds slow. |
| run the site for local development with automatic rebuilding of Javascript and sass without css sourcemaps | `npm run watch` then visit `http://localhost:3001/`. You may also set `buildtype` and `NODE_ENV` though setting `NODE_ENV` to production will make incremental builds slow. |
| run the site for local development with automatic rebuilding of Javascript and sass with css sourcemaps| `npm run watch:css-sourcemaps` then visit `http://localhost:3001/`. You may also set `buildtype` and `NODE_ENV` though setting `NODE_ENV` to production will make incremental builds slow. |
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth adding a note about the CSS source maps being heavy on memory and that's why we separate them out.

@rianfowler
Copy link
Author

rianfowler commented Jan 18, 2019

Thanks- I'll take a look at this line @ncksllvn. It doesn't look like we're using this plugin in the local build (isOptimized is false during local build).

I'm not familiar with the difference between plugins and loaders and if they're mutually exclusive (e.g we would use this in place of css-loader sourcemaps).

@va-bot va-bot temporarily deployed to vetsgov-pr-9415 January 21, 2019 15:49 Inactive
@va-bot va-bot temporarily deployed to vetsgov-pr-9415 January 21, 2019 15:52 Inactive
…ts-website into build/16380-make-dev-css-sourcemaps-optional
@va-bot va-bot temporarily deployed to vetsgov-pr-9415 January 21, 2019 16:06 Inactive
@va-bot va-bot temporarily deployed to master/build/16380-make-dev-css-sourcemaps-optional January 21, 2019 16:24 Inactive
@rianfowler rianfowler merged commit f105ed1 into master Jan 21, 2019
@rianfowler rianfowler deleted the build/16380-make-dev-css-sourcemaps-optional branch January 21, 2019 16:59
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

4 participants