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

DEP Update dependencies for webpack config #53

Merged

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Nov 1, 2022

Updates dependencies, modernises the build config, and reduces repetition of config.

Note that modules can still use the old config format, which will be useful for projects and third-party modules who want to make minimal changes, but the *WebpackConfig classes handle a lot of the boilerplate which makes it easier for us to make wide-sweeping changes later down the line if we want/need to (e.g. if webpack changes the syntax for some of the config, we just need to change it in this library).

Parent issue

/**
* A base class for dynamically generating webpack config using Silverstripe default settings
*/
module.exports = class BaseWebpackConfig {
Copy link
Member Author

Choose a reason for hiding this comment

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

This file allows us to simplify the js and css webpack config in individual modules a lot by reducing repetitive patterns - this way module config can rely on these sane defaults, with the ability to easily merge custom config in or replace config if necessary.

* Otherwise you will get errors because webpack wants to create a js file for each css file. The js files
* aren't emitted thanks to the IgnoreEmitPlugin, but this happens after chunk validation.
*/
module.exports = class CssWebpackConfig extends BaseWebpackConfig {
Copy link
Member Author

Choose a reason for hiding this comment

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

99% of the time our modules will want this exact config for sass > css - any that don't can easily use functionality from BaseWebpackConfig to add their customisations.

/**
* Dynamically generates webpack config for transpiling javascript using Silverstripe default settings
*/
module.exports = class JavascriptWebpackConfig extends BaseWebpackConfig {
Copy link
Member Author

Choose a reason for hiding this comment

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

99% of the time our modules will want this exact config for js - any that don't can easily use functionality from BaseWebpackConfig to add their customisations.

css/modules.js Show resolved Hide resolved
* @param {string} SRC The path to the source scss files
* @param {string} ROOT The path to the root of the project, this is so we can scope for
* silverstripe-admin variables.scss
* @param {boolean} useStyle Determines whether to use the style loader or extract text plugin
Copy link
Member Author

Choose a reason for hiding this comment

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

This option was added in #10 but doesn't seem to have ever been used. I'm removing it here both to reduce complexity and because style-loader doesn't work the same way it used to, and wants to put js inside the output css files.

Comment on lines -20 to +21
['env', { modules: false }],
'react',
],
plugins: [
'transform-object-rest-spread',
['@babel/preset-env', { useBuiltIns: 'usage', corejs: '3', modules: 'commonjs' }],
'@babel/preset-react',
Copy link
Member Author

Choose a reason for hiding this comment

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

env and react are replaced with the modern @babel/preset-env and @babel/preset-react respectively. The new options are necessary, since babel's polyfills are replaced with core-js.

The transform-object-rest-spread polyfill is no longer needed, as we don't support any browsers that don't natively support the functionality it polyfills.

*/
module.exports = (ENV) => {
const uglifyPlugin = (ENV === 'production')
Copy link
Member Author

Choose a reason for hiding this comment

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

Comments are stripped by default, and the UglifyJsPlugin no longer exists.

@@ -27,8 +16,6 @@ module.exports = (ENV) => {
NODE_ENV: JSON.stringify(ENV || 'development'),
},
}),
new webpack.NamedModulesPlugin(),
Copy link
Member Author

@GuySartorelli GuySartorelli Nov 29, 2022

Choose a reason for hiding this comment

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

This plugin no longer exists but is also no longer needed. It seems to have been related to a combination of making debugging easier and caching. I haven't noticed any difference since removing it.
For prosterity's sake I'll point out that if we do want this back in the future we can use the following as per the docs:

module.exports = {
  //...
  optimization: {
    moduleIds: 'named',
  },
};

@@ -1,13 +1,9 @@
{
"name": "@silverstripe/webpack-config",
"version": "1.7.0",
"version": "2.0.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

We'll be releasing a new major for this.

Comment on lines -8 to -10
"bin": {
"webpack": "./node_modules/.bin/webpack",
"sass-lint": "./node_modules/.bin/sass-lint"
Copy link
Member Author

Choose a reason for hiding this comment

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

bin isn't needed since there are no scripts in this file.

@GuySartorelli GuySartorelli marked this pull request as ready for review November 29, 2022 02:28
@GuySartorelli
Copy link
Member Author

GuySartorelli commented Nov 29, 2022

For a usage example, see this webpack config file which comes from this commit from an in-progress PR

/**
* If we are splitting a vendor bundle, make sure all entries "depend on" it as appopriate.
*/
#splitVendorFromEntries() {
Copy link
Member Author

Choose a reason for hiding this comment

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

@GuySartorelli GuySartorelli changed the base branch from master to 2 November 29, 2022 03:15
@GuySartorelli GuySartorelli marked this pull request as draft November 29, 2022 04:38
@GuySartorelli GuySartorelli force-pushed the pulls/master/new-major branch 5 times, most recently from 3caa6e4 to 17e1b17 Compare December 2, 2022 00:12
@GuySartorelli GuySartorelli marked this pull request as ready for review December 2, 2022 01:10
Comment on lines +123 to +127
// Don't include the exports provided by the current module
if (module && exports[module] !== undefined) {
delete exports[module];
}
Copy link
Member Author

@GuySartorelli GuySartorelli Dec 2, 2022

Choose a reason for hiding this comment

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

The way exports behaves seems to have changed - if the file which is exporting these also has them as external it won't actually include them in the bundle, so they don't get exported and there are errors all over the place. So we have to exclude those from external where appropriate.

js/externals.js Outdated
// bundles/bundle.js aliases
config: 'Config', // alias for lib/Config
// bundles/vendor.js
'@apollo/client': 'ApolloClient',
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
'@apollo/client': 'ApolloClient',
// This can't be exposed - see https://github.com/webpack-contrib/expose-loader/issues/188
// '@apollo/client': 'ApolloClient',

@GuySartorelli GuySartorelli marked this pull request as draft December 2, 2022 04:55
@GuySartorelli GuySartorelli force-pushed the pulls/master/new-major branch 2 times, most recently from fe6f4d8 to b1a20e9 Compare December 2, 2022 04:58
@GuySartorelli GuySartorelli marked this pull request as ready for review December 2, 2022 04:58
css/modules.js Show resolved Hide resolved
README.md Outdated

To use all of the default configuration for javascript transpilation, instantiate a new `JavascriptWebpackConfig` object.

This class's constructor takes a `name` string argument (used in the weback console output and for debugging) and a `PATHS` object. It also has a third argument (`moduleName`) which is only needed for core and supported Silverstripe modules and should be set to the name of the module (e.g. `silverstripe/admin`).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This class's constructor takes a `name` string argument (used in the weback console output and for debugging) and a `PATHS` object. It also has a third argument (`moduleName`) which is only needed for core and supported Silverstripe modules and should be set to the name of the module (e.g. `silverstripe/admin`).
This class's constructor takes a `name` string argument (used in the webpack console output and for debugging) and a `PATHS` object. It also has a third argument (`moduleName`) which is only needed for core and supported Silverstripe modules and should be set to the name of the module (e.g. `silverstripe/admin`).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

README.md Outdated

#### Example

This is a minimal example of how to build a webpack configuration array for Silverstripe modules without using the abstraction classes. It produces the exact same configuarion (and therefore the same output files) as the example [using the abstractions](#using-the-abstractions) above.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This is a minimal example of how to build a webpack configuration array for Silverstripe modules without using the abstraction classes. It produces the exact same configuarion (and therefore the same output files) as the example [using the abstractions](#using-the-abstractions) above.
This is a minimal example of how to build a webpack configuration array for Silverstripe modules without using the abstraction classes. It produces the exact same configuration (and therefore the same output files) as the example [using the abstractions](#using-the-abstractions) above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@emteknetnz emteknetnz merged commit 071271b into silverstripe:2 Dec 5, 2022
@emteknetnz emteknetnz deleted the pulls/master/new-major branch December 5, 2022 02:11
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