Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 1, 2019
Merged

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

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7471b86
fix(gatsby-plugin-netlify-cms): exclude node_modules from cms builds
erquhart Jun 27, 2019
9234f46
make it simpler
wardpeet Jun 28, 2019
aedfe94
mistakes were made
wardpeet Jun 28, 2019
50215de
make checks more robust
wardpeet Jul 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 53 additions & 54 deletions packages/gatsby-plugin-netlify-cms/src/gatsby-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,23 @@
import path from "path"
import { get, mapValues, isPlainObject, trim } from "lodash"
import { mapValues, isPlainObject, trim } from "lodash"
import webpack from "webpack"
import HtmlWebpackPlugin from "html-webpack-plugin"
import HtmlWebpackExcludeAssetsPlugin from "html-webpack-exclude-assets-plugin"
import MiniCssExtractPlugin from "mini-css-extract-plugin"
// TODO: swap back when https://github.com/geowarin/friendly-errors-webpack-plugin/pull/86 lands
import FriendlyErrorsPlugin from "@pieh/friendly-errors-webpack-plugin"

/**
* Deep mapping function for plain objects and arrays. Allows any value,
* including an object or array, to be transformed.
*/
// Deep mapping function for plain objects and arrays. Allows any value,
// including an object or array, to be transformed.
function deepMap(obj, fn) {
/**
* If the transform function transforms the value, regardless of type,
* return the transformed value.
*/
// If the transform function transforms the value, regardless of type, return
// the transformed value.
const mapped = fn(obj)
if (mapped !== obj) {
return mapped
}

/**
* Recursively deep map arrays and plain objects, otherwise return the value.
*/
// Recursively deep map arrays and plain objects, otherwise return the value.
if (Array.isArray(obj)) {
return obj.map(value => deepMap(value, fn))
}
Expand All @@ -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) {
Copy link
Contributor Author

@erquhart erquhart Jun 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@DSchau DSchau Jul 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@erquhart do you have an example?

var a = 'node_modules' // or var a = ''

if (!a || !a.test) {
  // we will be in this block
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return value
}

// when javascript we exclude node_modules
if (value.type === `javascript/auto` && value.exclude) {
return {
...value,
exclude: new RegExp(
[value.exclude.source, `node_modules|bower_components`].join(`|`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

),
}
}

// Manually swap `style-loader` for `MiniCssExtractPlugin.loader`.
// `style-loader` is only used in development, and doesn't allow us to pass
// the `styles` entry css path to Netlify CMS.
if (
typeof value.loader === `string` &&
value.loader.includes(`style-loader`)
) {
return {
...value,
loader: MiniCssExtractPlugin.loader,
}
}

return value
}

exports.onCreateDevServer = ({ app, store }, { publicPath = `admin` }) => {
const { program } = store.getState()
const publicPathClean = trim(publicPath, `/`)
Expand All @@ -49,7 +75,7 @@ exports.onCreateDevServer = ({ app, store }, { publicPath = `admin` }) => {
}

exports.onCreateWebpackConfig = (
{ store, stage, getConfig, plugins, pathPrefix },
{ store, stage, getConfig, plugins, pathPrefix, loaders },
{
modulePath,
publicPath = `admin`,
Expand Down Expand Up @@ -79,26 +105,11 @@ exports.onCreateWebpackConfig = (
path: path.join(program.directory, `public`, publicPathClean),
},
module: {
/**
* Manually swap `style-loader` for `MiniCssExtractPlugin.loader`.
* `style-loader` is only used in development, and doesn't allow us to
* pass the `styles` entry css path to Netlify CMS.
*/
rules: deepMap(gatsbyConfig.module.rules, value => {
if (
typeof get(value, `loader`) === `string` &&
value.loader.includes(`style-loader`)
) {
return { ...value, loader: MiniCssExtractPlugin.loader }
}
return value
}),
rules: deepMap(gatsbyConfig.module.rules, replaceRule),
},
plugins: [
/**
* Remove plugins that either attempt to process the core Netlify CMS
* application, or that we want to replace with our own instance.
*/
// Remove plugins that either attempt to process the core Netlify CMS
// application, or that we want to replace with our own instance.
...gatsbyConfig.plugins.filter(
plugin =>
![`MiniCssExtractPlugin`, `GatsbyWebpackStatsExtractor`].find(
Expand All @@ -122,49 +133,37 @@ exports.onCreateWebpackConfig = (
},
}),

/**
* Use a simple filename with no hash so we can access from source by
* path.
*/
// Use a simple filename with no hash so we can access from source by
// path.
new MiniCssExtractPlugin({
filename: `[name].css`,
}),

/**
* Auto generate CMS index.html page.
*/
// Auto generate CMS index.html page.
new HtmlWebpackPlugin({
title: htmlTitle,
chunks: [`cms`],
excludeAssets: [/cms.css/],
}),

/**
* Exclude CSS from index.html, as any imported styles are assumed to be
* targeting the editor preview pane. Uses `excludeAssets` option from
* `HtmlWebpackPlugin` config.
*/
// Exclude CSS from index.html, as any imported styles are assumed to be
// targeting the editor preview pane. Uses `excludeAssets` option from
// `HtmlWebpackPlugin` config.
new HtmlWebpackExcludeAssetsPlugin(),

/**
* Pass in needed Gatsby config values.
*/
// Pass in needed Gatsby config values.
new webpack.DefinePlugin({
__PATH__PREFIX__: pathPrefix,
CMS_PUBLIC_PATH: JSON.stringify(publicPath),
}),
].filter(p => p),

/**
* Remove common chunks style optimizations from Gatsby's default
* config, they cause issues for our pre-bundled code.
*/
// Remove common chunks style optimizations from Gatsby's default
// config, they cause issues for our pre-bundled code.
mode: stage === `develop` ? `development` : `production`,
optimization: {
/**
* Without this, node can get out of memory errors
* when building for production.
*/
// Without this, node can get out of memory errors when building for
// production.
minimizer: stage === `develop` ? [] : gatsbyConfig.optimization.minimizer,
},
devtool: stage === `develop` ? `cheap-module-source-map` : `source-map`,
Expand Down