-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Regroup non-static files #6285
Regroup non-static files #6285
Changes from 2 commits
4803d61
582fb41
50dd934
0986a67
7ed8da9
e1b8be9
77371b9
59cf18a
a9bc9b8
fdcf23a
2acc8b9
912751e
0513a6c
1041f85
50cb0bb
37d007a
0add064
f5d143b
eb17042
b8c8040
f9f4cd6
06e7360
93c3554
e02df8f
6ec8c70
11206e4
150d862
3637083
c2bdb2d
6f789f6
366bd97
972fb6b
4042ae1
695cb68
699e5ca
eed6616
d8a8377
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
}, | ||
"globals": { | ||
"__PATH_PREFIX__": false, | ||
"__GROUP_FILES__": false, | ||
"___emitter": false | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,11 @@ const createElement = React.createElement | |
export default (pagePath, callback) => { | ||
const pathPrefix = `${__PATH_PREFIX__}/` | ||
|
||
// If enabled, group files in their respective folders otherwise generated | ||
// non-static files will be left in the root of the output directory. | ||
const pathToGroupStyles = __GROUP_FILES__ ? `styles/` : `` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @KyleAMathews agreed. The original names were based off variables in the code. +1 for saving all the bytes. |
||
const pathToGroupScripts = __GROUP_FILES__ ? `scripts/` : `` | ||
|
||
let bodyHtml = `` | ||
let headComponents = [] | ||
let htmlAttributes = {} | ||
|
@@ -193,6 +198,7 @@ export default (pagePath, callback) => { | |
const scripts = scriptsAndStyles.filter( | ||
script => script.name && script.name.endsWith(`.js`) | ||
) | ||
|
||
const styles = scriptsAndStyles.filter( | ||
style => style.name && style.name.endsWith(`.css`) | ||
) | ||
|
@@ -221,7 +227,7 @@ export default (pagePath, callback) => { | |
as="script" | ||
rel={script.rel} | ||
key={script.name} | ||
href={urlJoin(pathPrefix, script.name)} | ||
href={urlJoin(pathPrefix, pathToGroupScripts, script.name)} | ||
/> | ||
) | ||
}) | ||
|
@@ -252,7 +258,7 @@ export default (pagePath, callback) => { | |
as="style" | ||
rel={style.rel} | ||
key={style.name} | ||
href={urlJoin(pathPrefix, style.name)} | ||
href={urlJoin(pathPrefix, pathToGroupStyles, style.name)} | ||
/> | ||
) | ||
} else { | ||
|
@@ -262,7 +268,7 @@ export default (pagePath, callback) => { | |
data-href={urlJoin(pathPrefix, style.name)} | ||
dangerouslySetInnerHTML={{ | ||
__html: fs.readFileSync( | ||
join(process.cwd(), `public`, style.name), | ||
join(process.cwd(), `public`, pathToGroupStyles, style.name), | ||
`utf-8` | ||
), | ||
}} | ||
|
@@ -291,7 +297,9 @@ export default (pagePath, callback) => { | |
// Filter out prefetched bundles as adding them as a script tag | ||
// would force high priority fetching. | ||
const bodyScripts = scripts.filter(s => s.rel !== `prefetch`).map(s => { | ||
const scriptPath = `${pathPrefix}${JSON.stringify(s.name).slice(1, -1)}` | ||
const scriptPath = `${pathPrefix}${pathToGroupScripts}${JSON.stringify( | ||
s.name | ||
).slice(1, -1)}` | ||
return <script key={scriptPath} src={scriptPath} async /> | ||
}) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,6 +86,25 @@ module.exports = async ( | |
return hmrBasePath + hmrSuffix | ||
} | ||
|
||
function getOutputPaths() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was duplicate code in the webpack output configs, so I moved it into a function that also handles the grouping of files. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
const path = program.groupFiles | ||
? directoryPath(`public/scripts`) | ||
: directoryPath(`public`) | ||
|
||
let publicPath = program.prefixPaths | ||
? `${store.getState().config.pathPrefix}/` | ||
: `/` | ||
|
||
if (program.groupFiles) { | ||
publicPath = `${publicPath}scripts/` | ||
} | ||
|
||
return { | ||
path, | ||
publicPath, | ||
} | ||
} | ||
|
||
debug(`Loading webpack config for stage "${stage}"`) | ||
function getOutput() { | ||
switch (stage) { | ||
|
@@ -109,24 +128,20 @@ module.exports = async ( | |
// A temp file required by static-site-generator-plugin. See plugins() below. | ||
// Deleted by build-html.js, since it's not needed for production. | ||
return { | ||
path: directoryPath(`public`), | ||
filename: `render-page.js`, | ||
libraryTarget: `umd`, | ||
library: `lib`, | ||
umdNamedDefine: true, | ||
globalObject: `this`, | ||
publicPath: program.prefixPaths | ||
? `${store.getState().config.pathPrefix}/` | ||
: `/`, | ||
path: getOutputPaths().path, | ||
publicPath: getOutputPaths().publicPath, | ||
} | ||
case `build-javascript`: | ||
return { | ||
filename: `[name]-[chunkhash].js`, | ||
chunkFilename: `[name]-[chunkhash].js`, | ||
path: directoryPath(`public`), | ||
publicPath: program.prefixPaths | ||
? `${store.getState().config.pathPrefix}/` | ||
: `/`, | ||
path: getOutputPaths().path, | ||
publicPath: getOutputPaths().publicPath, | ||
} | ||
default: | ||
throw new Error(`The state requested ${stage} doesn't exist.`) | ||
|
@@ -167,12 +182,14 @@ module.exports = async ( | |
plugins.moment(), | ||
|
||
// Add a few global variables. Set NODE_ENV to production (enables | ||
// optimizations for React) and what the link prefix is (__PATH_PREFIX__). | ||
// optimizations for React), what the link prefix is (__PATH_PREFIX__), | ||
// and __GROUP_FILES__ to organize output files. | ||
plugins.define({ | ||
"process.env": processEnv(stage, `development`), | ||
__PATH_PREFIX__: JSON.stringify( | ||
program.prefixPaths ? store.getState().config.pathPrefix : `` | ||
), | ||
__GROUP_FILES__: !!program.groupFiles, | ||
}), | ||
] | ||
|
||
|
@@ -186,12 +203,12 @@ module.exports = async ( | |
clearConsole: false, | ||
compilationSuccessInfo: { | ||
messages: [ | ||
`You can now view your site in the browser running at ${program.ssl ? `https` : `http`}://${ | ||
program.host | ||
}:${program.port}`, | ||
`Your graphql debugger is running at ${program.ssl ? `https` : `http`}://${program.host}:${ | ||
program.port | ||
}/___graphql`, | ||
`You can now view your site in the browser running at ${ | ||
program.ssl ? `https` : `http` | ||
}://${program.host}:${program.port}`, | ||
`Your graphql debugger is running at ${ | ||
program.ssl ? `https` : `http` | ||
}://${program.host}:${program.port}/___graphql`, | ||
], | ||
}, | ||
}), | ||
|
@@ -227,7 +244,6 @@ module.exports = async ( | |
) | ||
} | ||
} | ||
|
||
const webpackStats = { | ||
...stats.toJson({ all: false, chunkGroups: true }), | ||
assetsByChunkName: assets, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a flag to enable grouping of files.
gatsby build --group-files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a flag? We try to avoid adding config wherever possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KyleAMathews -- my bad!
What's the best way to add a "feature flag" for this? I tried looking through the code but couldn't grasp it right away. Is there a standard way to handle this?
I was trying to address this comment
#6285 (review)
The flag would be temporary for people to try it out and then enabled by default after some time. After that the config option would be removed. The purpose is to provide a smoother transition with a testing phase before implementing a potential breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, there shouldn't be any plugins which directly operate on outputted css/js so I think this change is pretty safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m-allanson @pieh @jquense can you think of any reason we shouldn't just make this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
w/o the flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the flags the code would be a lot cleaner. Less config options exposed to users and I can remove ifs, globals, and other logic from this PR. I'm in favour of removing the flag if it only impacts 1-2 plugins.