fix(gatsby-plugin-netlify-cms): exclude node_modules from cms #15191

erquhart · 2019-06-27T20:16:30Z

Description

#14111 caused a regression for gatsby-plugin-netlify-cms, which reuses a modified version of Gatsby's final webpack config and requires that node_modules be excluded from all builds. This PR adds the exclusion directly to the plugin.

Related to #15190

moonmeister · 2019-06-27T20:23:34Z

@erquhart Can you give some context, maybe the original issue or a explanation of the regression? I just ran into some issues while updating my site but was able to get it to build and deploy. It appears to be working. see #15190 for my troubles.

P.S. I'm mostly just curious and want to understand...not saying you're incorrect.

wardpeet

Seems like I can't fix it from inside gatsby.

I inlined some of your replace functions, didn't seemed necessary to abstract it away. made it harder to read for me.

Awesome work! when everything is green let me merge and get this patched. Thank you for making gatsby better! 👍

erquhart · 2019-06-28T13:25:21Z

@wardpeet thanks for the edits! I think I might see a couple of issues, let me give a quick look before you merge, I'll be at my machine soon.

erquhart

@wardpeet I added a few comments - the gist is that, we should be using safe methods like lodash.get and castArray here. This plugin is coloring outside the lines of Gatsby's API, constructing a Webpack config from the raw received config, and there are no guarantees as to what that object will look like.

Performance is also not an issue as this whole thing runs more or less instantaneously, and only once per build.

erquhart · 2019-06-28T14:24:11Z

packages/gatsby-plugin-netlify-cms/src/gatsby-node.js

@@ -33,6 +27,38 @@ function deepMap(obj, fn) {
  return obj
 }

+function replaceRule(value) {
+  // If `value` does not have a `test` property, it isn't a rule object.
+  if (!value || !value.test) {


value could be a string - we're recursing the entire received object, in which case !value.test would throw. Rather than first making sure it's an object and risking some other unforeseen issue, we should just go bulletproof with lodash.get.

if test is a string it will give a falsy value (undefined) when value.test is used. So no harm here, thanks for bringing it up to my attention.

No, value could be a string. This will throw for some users.

@erquhart do you have an example?

var a = 'node_modules' // or var a = '' if (!a || !a.test) { // we will be in this block }

Hmm not sure why I was thinking that would throw as if value is undefined. I know I ran into issues deep mapping the webpack config and needed safe getters, but you're right, this wouldn't be an issue at this line.

erquhart · 2019-06-28T14:25:21Z

packages/gatsby-plugin-netlify-cms/src/gatsby-node.js

+    return {
+      ...value,
+      exclude: new RegExp(
+        [value.exclude.source, `node_modules|bower_components`].join(`|`)


We don't know for sure that the exclude is and will continue to be a RegExp, it could easily be an array. castArray is the safe route, its guaranteed not to fail, we always know what we're getting, and the output is equally compatible with Webpack.

I'm checking that it's a regex now. If people have a custom js rule they should make sure to exclude node_modules themselves.

erquhart · 2019-06-28T14:36:35Z

@moonmeister ah, your issue is exactly what this addresses, I'll comment there.

erquhart · 2019-06-28T22:52:01Z

@wardpeet sorry to bug! Any chance of getting this in before the weekend? Bit of a showstopper for affected users.

Gatsby@2.11.0 Build failure gatsbyjs/gatsby#15190 (comment) > gatsby-plugin-netlify-cms runs an independent Webpack build within > Gatsby's Webpack build (insert "yo dawg" reference here) using a > modified version of the Gatsby Webpack config. Gatsby 2.11.0 no longer > excludes node_modules, which causes the plugin's Webpack build to attempt > processing the Netlify CMS modules with Babel. The modules are massive > and also prebuilt, so it will (and should) crash your system. > The PR I raised fix(gatsby-plugin-netlify-cms): exclude node_modules from cms builds gatsbyjs/gatsby#15191 > just adds that exclusion back in to the plugin's Webpack config. > Please don't override your node max file size limit to fix this! feat(gatsby): enable babel-loader for all dependencies gatsbyjs/gatsby#14111

Jamamuuga · 2019-06-29T01:22:28Z

Will any additional changes be needed for a developer who uses the plugin & uses Yarn's PnP feature?

erquhart · 2019-06-29T14:53:54Z

Update: comments on #15190 show that this isn't limited to the Netlify CMS plug-in. The appropriate fix to that issue may supercede this PR.

_{Sent with GitHawk}

wardpeet

Updated PR, thanks @erquhart for bringing those thinks to my attention

@moonmeister, it's just an out of memory as the cms.js bundle is about 3mb if not mistaken and a bit much to store in memory it seems.

wardpeet · 2019-07-01T05:43:36Z

packages/gatsby-plugin-netlify-cms/src/gatsby-node.js

@@ -33,6 +27,38 @@ function deepMap(obj, fn) {
  return obj
 }

+function replaceRule(value) {
+  // If `value` does not have a `test` property, it isn't a rule object.
+  if (!value || !value.test) {


if test is a string it will give a falsy value (undefined) when value.test is used. So no harm here, thanks for bringing it up to my attention.

wardpeet · 2019-07-01T05:46:16Z

packages/gatsby-plugin-netlify-cms/src/gatsby-node.js

+    return {
+      ...value,
+      exclude: new RegExp(
+        [value.exclude.source, `node_modules|bower_components`].join(`|`)


I'm checking that it's a regex now. If people have a custom js rule they should make sure to exclude node_modules themselves.

wardpeet

Thank you for getting this fixed @erquhart! ❤️

erquhart · 2019-07-01T15:36:46Z

@wardpeet you misread my comment: value may be a string, in which case value.test will throw. I also commented that a proper fix to #15190 would likely supercede this PR, which appears to have happened in #15270. I recommend reverting this PR.

erquhart · 2019-07-01T17:26:04Z

Took another look based on @DSchau's comment on the review, and you're right, value wouldn't throw. I do still think this should be reverted since the bug it addressed has itself been reverted.

rdela · 2019-07-06T19:40:39Z

@wardpeet @erquhart In case it helps figure any of this out, this repo that uses Netlify CMS is still broken under ~~2.13.6~~ 2.13.9, but everything is fine under 2.10.5. See master branch vs rdela/voidcluster#54; build failure log at ~~https://app.netlify.com/sites/voidcluster/deploys/5d20f59e9f191b000a971e08~~
https://app.netlify.com/sites/voidcluster/deploys/5d242cc7332e50bd7113ca72

erquhart requested a review from a team as a code owner June 27, 2019 20:16

fix(gatsby-plugin-netlify-cms): exclude node_modules from cms builds

7471b86

erquhart force-pushed the cms-build-fix branch from 42eebf4 to 7471b86 Compare June 27, 2019 20:26

wardpeet added 2 commits June 28, 2019 15:01

make it simpler

9234f46

mistakes were made

aedfe94

wardpeet previously approved these changes Jun 28, 2019

View reviewed changes

wardpeet mentioned this pull request Jun 28, 2019

Gatsby@2.11.0 Build failure (JavaScript heap out of memory) #15190

Closed

erquhart commented Jun 28, 2019

View reviewed changes

make checks more robust

50215de

wardpeet dismissed their stale review via 50215de July 1, 2019 05:45

wardpeet reviewed Jul 1, 2019

View reviewed changes

wardpeet approved these changes Jul 1, 2019

View reviewed changes

wardpeet changed the title ~~fix(gatsby-plugin-netlify-cms): exclude node_modules from cms builds~~ fix(gatsby-plugin-netlify-cms): exclude node_modules from cms Jul 1, 2019

wardpeet merged commit a767854 into gatsbyjs:master Jul 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gatsby-plugin-netlify-cms): exclude node_modules from cms #15191

fix(gatsby-plugin-netlify-cms): exclude node_modules from cms #15191

erquhart commented Jun 27, 2019 •

edited

moonmeister commented Jun 27, 2019

wardpeet left a comment

erquhart commented Jun 28, 2019

erquhart left a comment •

edited

erquhart Jun 28, 2019 •

edited

wardpeet Jul 1, 2019

erquhart Jul 1, 2019

DSchau Jul 1, 2019 •

edited

erquhart Jul 1, 2019

erquhart Jun 28, 2019

wardpeet Jul 1, 2019

erquhart commented Jun 28, 2019

erquhart commented Jun 28, 2019

Jamamuuga commented Jun 29, 2019

erquhart commented Jun 29, 2019

wardpeet left a comment

wardpeet Jul 1, 2019

wardpeet Jul 1, 2019

wardpeet left a comment

erquhart commented Jul 1, 2019

erquhart commented Jul 1, 2019

rdela commented Jul 6, 2019 •

edited

fix(gatsby-plugin-netlify-cms): exclude node_modules from cms #15191

fix(gatsby-plugin-netlify-cms): exclude node_modules from cms #15191

Conversation

erquhart commented Jun 27, 2019 • edited

Description

moonmeister commented Jun 27, 2019

wardpeet left a comment

Choose a reason for hiding this comment

erquhart commented Jun 28, 2019

erquhart left a comment • edited

Choose a reason for hiding this comment

erquhart Jun 28, 2019 • edited

Choose a reason for hiding this comment

wardpeet Jul 1, 2019

Choose a reason for hiding this comment

erquhart Jul 1, 2019

Choose a reason for hiding this comment

DSchau Jul 1, 2019 • edited

Choose a reason for hiding this comment

erquhart Jul 1, 2019

Choose a reason for hiding this comment

erquhart Jun 28, 2019

Choose a reason for hiding this comment

wardpeet Jul 1, 2019

Choose a reason for hiding this comment

erquhart commented Jun 28, 2019

erquhart commented Jun 28, 2019

Jamamuuga commented Jun 29, 2019

erquhart commented Jun 29, 2019

wardpeet left a comment

Choose a reason for hiding this comment

wardpeet Jul 1, 2019

Choose a reason for hiding this comment

wardpeet Jul 1, 2019

Choose a reason for hiding this comment

wardpeet left a comment

Choose a reason for hiding this comment

erquhart commented Jul 1, 2019

erquhart commented Jul 1, 2019

rdela commented Jul 6, 2019 • edited

erquhart commented Jun 27, 2019 •

edited

erquhart left a comment •

edited

erquhart Jun 28, 2019 •

edited

DSchau Jul 1, 2019 •

edited

rdela commented Jul 6, 2019 •

edited