Skip to content
This repository has been archived by the owner on May 2, 2022. It is now read-only.

Switch to Webpack #111

Merged
merged 20 commits into from
Apr 12, 2019
Merged

Switch to Webpack #111

merged 20 commits into from
Apr 12, 2019

Conversation

samikeijonen
Copy link
Contributor

@samikeijonen samikeijonen commented Mar 25, 2019

Fixes #110

I'll add my notes tomorrow, it's getting too late.

Relates to issue #110.

@samikeijonen
Copy link
Contributor Author

samikeijonen commented Mar 26, 2019

@timwright12 and others, here are my notes and questions.

proxyUrl for browserSync

Currently we have proxyUrl in package.json file. In PR I am using it from config/webpack.settings.jsmainly because there are others browserSync relates settings.

Can project-scaffold rename proxyUrl from config/webpack.settings.js, or should we keep it in package.json?

Entry names

There is one entry name different. styleguide-style use to be styleguide but Webpack doesn't accept same entry for JS and CSS file.

One NPM script added

I Added one new NPM script: npm run dev. All the other NPM scripts are the same.

Note that there are less NPM packages with this setup.

Added postcss.config.js

For clarity I have added PostCSS config in it's own postcss.config.js file. Should we have postcss.config.js in config folder?

Rename *.min files without min

For example style.min.css is now always style.css. The reason for this is that npm run dev and npm run watch doesn't minify CSS or JS. This makes dev process a little bit faster.

And changing file names for prod and dev doesn't feel necessary for me.

Sourcemaps

Sourcemaps are only added on dev mode inside the files itself. In other words there are no separate sourcemap files anymore.

cssnano

We need to check cssnano settings just in case, what default holds. Settings can be found in postcss.config.js. I have used the same settings as before but I think I added discardComments.

Terser

Switch over to Terser compressor which seem to have better ES6+ support.

I'd appreciate help on testing cssnano settings.

Update docs

Needs to update readme about used NPM packages.

And add documentation about basic principles where to find config files and what they mean.

Webpack plugins to consider

I didn't add any Imagemin Plugins but this is something to consider.

Webpack Manifest Plugin is also nice but I'm not sure should it be in theme-scaffold. Bust caching can be done in different ways.

I also didn't add any Webpack cache plugins. They would make build process faster but I have read that cache might come to Webpack 5. I'd vote no for cache plugins at this point.

NMP audit

npm audit gives one browser-sync related note mention in this Braces issue.

General notes

There is some tweaks and cleaning for sure. Now it's good time to question everything on build process 😃

Testing

All the testing in different environments would be helpful.

  • Run all npm commands.
  • For testing npm run watch you need to manually change BrowserSyncConfig.proxy in config/webpack.settings.js.

@timwright12
Copy link
Contributor

I'm a little backed up from pre-PTO right now, but I'll make a note to review this Monday when I'm back.

@dkoo
Copy link
Contributor

dkoo commented Mar 29, 2019

Just wanted to note that I tested this branch and all commands and features work as expected. One thing I might request is that npm run start also run composer install because otherwise the lint and lint-php commands will fail unless you manually run composer install, npm run build-release, or npm run lint-release first. This is also an issue with the master branch but we may as well fix it here, right?

Organization of the webpack config files makes sense and is quite clean. Performance seems great to me. Kudos to @samikeijonen for pulling together this PR so quickly.

Copy link
Contributor

@timwright12 timwright12 left a comment

Choose a reason for hiding this comment

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

It doesn't seem like the stylelinting or the estlinting is active here, can we add that in? The old/current one also ran --fix on eslint through webpack https://github.com/10up/theme-scaffold/blob/master/webpack.config.babel.js#L30

Also, let's make sure the readme gets updated. After we're good here, we'll need to add these changes into the plugin repo as well.

@samikeijonen
Copy link
Contributor Author

It doesn't seem like the stylelinting or the estlinting is active here, can we add that in?

@timwright12 Did you mean in Webpack build process?

@timwright12
Copy link
Contributor

@samikeijonen yeah, anywhere. I didn't see linting errors surface at all.

@samikeijonen
Copy link
Contributor Author

@timwright12 I added linting in the build process also.

  • eslint-loader for JS: 5e8a9ac.
  • stylelint-webpack-plugin for CSS: 2706788

I feel like CSS linting should happen in postcss.config.js without the plugin. But for some reason I didn't get it to work. I'll try again when I have more time.

@samikeijonen
Copy link
Contributor Author

@dkoo Added composer install in 2232295.

@samikeijonen
Copy link
Contributor Author

@timwright12 I have updated readme.md file as much I could think of at this point.

@timwright12
Copy link
Contributor

timwright12 commented Apr 10, 2019

@samikeijonen this looks good to me, can you port these over to the plugin repo? 10up/plugin-scaffold#84 we'll merge them at the same time

@timwright12 timwright12 merged commit 4168423 into 10up:master Apr 12, 2019
@samikeijonen samikeijonen deleted the feature/webpack branch April 12, 2019 13:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should we deprecate Gulp and focus on Webpack?
3 participants