Asset pipeline rewrite #4337

bcardarella · 2021-05-29T18:08:02Z

This is a comprehensive rewrite of the Phoenix asset pipeline. Closes #4328

LiveView has a similar pending PR: phoenixframework/phoenix_live_view#1474

Separated the single mono-js file into separate es6-style modules
Updated all js dependencies
Rewrote the webpack build pipeline for less complexity, modularity, and to take advantage of v5
Implemented eslint to enforce @chrismccord's js style rules

This does include breaking changes for existing apps with legacy Webpack configurations. Upgrade instructions:

copy package.json from installer/templates/phx_assets to overwrite the existing package.json into your app's assets/ directory.
cd assets && rm -rf node_modules && rm package-lock.json
npm install
copy webpack.config.js from installer/templates/phx_assets to overwrite the existing webpack.config.js in your assets/ directory

Note if you have any customizations in your existing webpack.config.js you should port them to the new config. This PR includes an update from Webpack v4 to Webpack v5 so there may be incompatibilities with any plugins, loaders, etc... that you were using web Webpack v4

By default the asset pipeline will import the es6 modules for Phoenix (as well as Phoenix LiveView). This means it is no longer building from pre-compiled JS. The result is that in dev mode the Phoenix and LiveView JS is no longer minified and no longer uglified. Debugging and stepping through code at runtime is now much easier. In production the JavaScript is minified and uglified as well as the CSS.

Babel has been removed from the build pipeline for apps. This was done because the target minimum browser version for Phoenix is being bumped up. Targeting IE11 is no longer in scope and we no longe need to include the Babel polyfill anymore. If you still require Babel for your own app's needs you can easily add Babel to your package.json and add the correct rules/loaders to webpack.config.js.

Sourcemaps are now included out of the box. The previous sourcemap implementation favored compilation speed over how useful they were. Now sourcemaps are generated and can be accessible in the developer console:

TODO for Sourcemaps: the naming of the asset files needs to be improved. They currently compile with their relative path names from the Phoenix and Phoenix LiveView directories (i.e. "js/phoenix/channel.js") This should be improved to just be "phoenix/channel.js"

ESlint was added with a set of rules that attempt to conform to @chrismccord's desired JS style. Currently the linter is not run with the test suite. You can run from the assets/ directory: npx eslint js and you can add --fix if you trust the linter to auto-fix anything that isn't compliant. I do think that the linter should be part of the test suite to help prevent any future style fix requests on PRs

There should not be any changes necessary to consuming app's app.js to consume the new es6 modules. I made sure to export the necessary modules properly.

TBD

Anyone not using webpack please weigh in on this. The changes I've made were with webpack in mind. I removed quite a bit that you may be taking for granted. Let's discuss that. While adding complexity to this PR can be done it isn't desirable. Webpack is being seen as the anointed asset pipeline solution for Phoneix. If you are using something else I believe a npm module to auto-configure what you need is the best step forward (i.e. creating a phoenix-rollup package) rather than adding to what Phoenix generates. However, if there is something missing that is an absolute blocker necessary to build in your own environment let's discuss what that is and how best to support.

TODO

CI if failing due to an attempt to copy the no longer existing babelrc file
LiveReload in dev mode attempts to find source maps... this likely isn't a problem for release as priv/static/phoenix.js should be a production build and not include any references to source maps.

bcardarella · 2021-05-30T06:04:56Z

@tozz eslint isn't enforced for consuming apps

tozz · 2021-05-30T06:15:07Z

@bcardarella Oh yeah, my bad, this is not the actual assets folder, it's just the naming of things that makes it confusing (things are named the same in source vs created project but are completely different) 😅

bcardarella · 2021-05-30T06:20:34Z

@tozz fwiw I think having the @phoenix scoped package name would be great but I have no idea if that is something the core team wants to take on

bcardarella · 2021-05-30T16:02:19Z

@chrismccord this test: https://github.com/phoenixframework/phoenix/blob/master/installer/test/phx_new_test.exs#L17-L22
is currently failing. Are the files in installer/templates/phx_static/ intended to be duplicates of what is generated into priv/static of the phoenix repo? I'm wondering if the copying of phoenix.js is still necessary with this PR

AndrewDryga · 2021-06-03T18:18:38Z

assets/.babelrc

@@ -2,4 +2,4 @@
    "presets": [
        "@babel/preset-env"
    ]
-}
+}


Is it intentional to have no newlines at the end of all files?

It isn't. I bet my editor did that. I'll correct

This should be fixed now. I added an eslint rule to catch it for the js files in the future, I don't think it will catch on this config files though.

tomciopp · 2021-06-08T21:43:31Z

@bcardarella Any thoughts on using webpack-merge for changing webpack configuration based on your current environment?

bcardarella · 2021-06-08T21:49:13Z

@tomciopp if I am understanding the README correctly, all this plugin is doing is a left->right object merge. So this would be used to write your own custom webpack config in a different file then merge into the default? If this is the case I don't really see a need for this. Why not just change what is generated from the template?

tomciopp · 2021-06-08T22:08:58Z

@bcardarella Sure you could do that, but webpack-merge is referenced in Webpack's documentation for production sites. If this is the way that other people using webpack expect to write configuration files I see no reason to reinvent the wheel, we should just follow that convention.

https://webpack.js.org/guides/production/

bcardarella · 2021-06-08T22:12:24Z

@tomciopp I see what you're saying but our goal here is to reduce complexity. For the majority of users they're unlikely to need to make changes to the webpack config. For those that want to they can add this plugin if they don't want to make many changes to the generated template. The asset pipeline got into its current state because things kept getting added and after a few conversations with @chrismccord to this point we're going to keep the default template as sparse and basic as possible. Nothing prevents people from adding whatever they'd like to customize away from the default.

tomciopp · 2021-06-08T22:26:33Z

@bcardarella

For the majority of users they're unlikely to need to make changes to the webpack config.

I'm not so sure about the validity of this statement, based on personal experience I've found it to be a necessity for every project. YMMV. I understand your point of reducing complexity, but if you are right and the majority of users don't change the configuration, shouldn't we optimize for people who need to? This way we aren't leaving people in hell if they aren't power users of webpack and need to make changes.

Perhaps a blog post would suffice for my opinionated guide for phoenix & webpack configuration for larger scale projects.

bcardarella · 2021-06-08T23:07:31Z

Perhaps a blog post [or a] guide for phoenix & webpack configuration for larger scale projects

Something like this would be great, but that's outside the scope of this PR. Recently SASS was removed and I'm sure there are some that were using it so including and a guide to things like that would be helpful too. Another guide would be on improving the build time of webpack. Prior to this PR there were some optimizations to handle slower builds but sacrificed source map fidelity. Everything is a trade off so giving some people pointers on these things would be good to have. However, I would caution that due to the complexity of webpack and the seemingly infinite permutations on webpack configs any effort on a guide of this type should probably have clearly defined boundaries.

snewcomer

This is amazing! Do we need the babel.config.json btw?

I also checked the asset size real quick. Currently phoenix.js is around 25kb. This PR it seems to have dropped to 20kb!

Also, what is the plan for versioning given the breaking changes?

snewcomer · 2021-06-12T04:05:07Z

assets/webpack.config.js

-          loader: 'babel-loader'
+      globalObject: 'this'
+    },
+    devtool: devMode ? 'source-map' : undefined,


snewcomer · 2021-06-13T03:36:45Z

installer/templates/phx_assets/package.json

@@ -11,16 +11,13 @@
    "topbar": "^0.1.4"<% end %>
  },
  "devDependencies": {
-    "@babel/core": "^7.0.0",
-    "@babel/preset-env": "^7.0.0",
-    "babel-loader": "^8.0.0",


snewcomer · 2021-06-13T03:37:05Z

installer/templates/phx_assets/webpack.config.js


 module.exports = (env, options) => {
  const devMode = options.mode !== 'production';
+  console.log(`CURRENT MODE: ${devMode}`)


I'm guessing this was intentional?

Good catch, will rm

snewcomer · 2021-06-13T03:39:49Z

installer/templates/phx_assets/webpack.config.js


  return {
+    resolve: {
+      modules: ['node_modules']
+    },


I think this is the default, but totally great that it is explicit!

snewcomer · 2021-06-13T03:51:32Z

package.json

-    "import": "./assets/js/phoenix.js",
-    "require": "./priv/static/phoenix.js"
+    ".": "./assets/js/phoenix/index.js",
+    "./": "./assets/js/phoenix/"


I'm assuming you found that you needed both?

The first one being import "phoenix";. Second one any subpath such as import "phoenix/longpoll";

if there is a pattern to do in 1 LOC I'm open to it

Implemented new pipeline with es6-ified modules and got mocha test suite green

cw789

Thanks for starting this topic. I took a quick look at the installer things, without testing or checking out this branch.

If any questions arises - please feel free.

cw789 · 2021-06-16T11:16:42Z

installer/templates/phx_assets/package.json

-    "terser-webpack-plugin": "^2.3.2",
-    "webpack": "4.41.5",
-    "webpack-cli": "^3.3.2"
+    "copy-webpack-plugin": "^9.0.0",


I do not see copy-webpack-plugin beeing used anymore within webpack.config.js.
Means it could be removed here.

But by removing copy-webpack-plugin I think in installer/lib/phx_new/single.ex copy path of robots.txt, phoenix.png and favicon.ico should be changed from assets/static/ to priv/static/.

With this change /priv/static should get removed in the .gitignore.

moving them over to priv/static might make sense, but it depends upon what experience @chrismccord wants here. If all web assets should be in the assets/ directory then copied over at build time I can bring back the copy rule.

We need to support copying assets from assets/static/ to priv/static, so we'lll need copy-webpack unless webpack can do something like this out of the box now. This allows folks to put all assets inside the assets directory, and build them into private/static, which is especially important for our digest task, which will digest the copied/built assets

Then it would be necessary to bring it back in.

I'm not aware of any simpler out of the box webpack solution.

One thing I once encountered but haven't evaluated in detail then,
copy-webpack-plugin caused to copy (or touch all copy files) if one of them canged. Means live reload reloads x times (or logged to reload).
But could be, that this was only true for a certain version of the updated plugin.

@cw789 I'm working on putting it back right now, the API changed with the version bump so I just have to ensure that it's doing everything we want.

cw789 · 2021-06-16T11:19:48Z

installer/templates/phx_assets/package.json

+    "copy-webpack-plugin": "^9.0.0",
+    "css-loader": "^5.2.6",
+    "css-minimizer-webpack-plugin": "^3.0.0",
+    "expose-loader": "3.0.0",


expose-loader not being used within webpack.config.js.

I left it in because @chrismccord and I were going back and forth on if the Phoenix constant should be exposed. He fell on the side that it wasn't necessary but I wanted to give the community a chance to provide feedback if for some reason a need for that constant is being overlooked.

To be clear, the Phoenix constant needs to be on window when the built js is loaded, for folks not using an asset builder and for the live reload project. If we can ditch expose-loader and do this directly then great, but we need to make sure it is exposed from the bundle.

It could technically be done without expose-loader but I think it s going to be some crazy rule writing. I would vote to keep the dependency to do this.

To be clear, keeping it on the internal webpack config, not the one in the installer template

works for me

chrismccord · 2021-06-22T14:20:05Z

❤️❤️❤️🐥🔥

gcauchon · 2021-06-22T17:29:00Z

@bcardarella @chrismccord Is there a reason assets/babel.config.json was renamed (to follow the .config.json convention) and not deleted?

bcardarella · 2021-06-22T18:24:21Z

@gcauchon babel is being used for the test suite for the js files

bcardarella · 2021-06-22T18:35:40Z

FWIW if anyone want to PR to have the js test suite use Webpack to resolve the es6 modules so we can drop Babel there too I'm all for it. I just gave up fighting JS that day I think.

bcardarella mentioned this pull request May 29, 2021

Rewrote the asset build pipeline phoenixframework/phoenix_live_view#1474

Closed

gcauchon mentioned this pull request Jun 1, 2021

Webpack 5 mirego/elixir-boilerplate#152

Merged

AndrewDryga reviewed Jun 3, 2021

View reviewed changes

cw789 mentioned this pull request Jun 8, 2021

Update to webpack v5 #4054

Closed

bcardarella force-pushed the asset-refactor branch from 15a36b6 to 8be7050 Compare June 8, 2021 23:12

snewcomer approved these changes Jun 13, 2021

View reviewed changes

bcardarella added 7 commits June 13, 2021 21:38

Asset pipeline rewrite

a5d2590

Implemented new pipeline with es6-ified modules and got mocha test suite green

build phoenix.js for production, strip sourcemaps

e6781a0

Remove references to babelrc

cecf9c5

Fix phx_new installer test by copying new phoenix.js file

f779a13

Ensure newline at EOF

efa74db

Rename babel config file name to match latest recommendation

213d5e0

Remove console.log

538c458

bcardarella force-pushed the asset-refactor branch from 8be7050 to 538c458 Compare June 14, 2021 01:38

cw789 suggested changes Jun 16, 2021

View reviewed changes

bcardarella added 2 commits June 21, 2021 15:25

restore the copy webpack rule

0f41d6e

Watch

b5fdc50

chrismccord merged commit b6b62aa into phoenixframework:master Jun 22, 2021

This was referenced Jun 22, 2021

Use Webpack v5.x for asset bundling #4310

Closed

Update js dependencies, prepare for webpack 5 #4146

Closed

simonmcconnell mentioned this pull request Jun 23, 2021

morphdom not found because phoenix_html imported w/o deps path phoenixframework/phoenix_live_view#1505

Closed

cw789 mentioned this pull request Jun 23, 2021

Improve asset pipline docs #4352

Merged

czardien mentioned this pull request Jul 2, 2021

Getting started guide raises an error: npm install, node-gyp python2 couldn't be found #4359

Closed

bcardarella deleted the asset-refactor branch January 1, 2024 15:45

Asset pipeline rewrite #4337

Asset pipeline rewrite #4337

Conversation

bcardarella commented May 29, 2021 • edited by mcrumm Loading

bcardarella commented May 30, 2021

tozz commented May 30, 2021

bcardarella commented May 30, 2021

bcardarella commented May 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomciopp commented Jun 8, 2021

bcardarella commented Jun 8, 2021

tomciopp commented Jun 8, 2021 • edited Loading

bcardarella commented Jun 8, 2021

tomciopp commented Jun 8, 2021

bcardarella commented Jun 8, 2021 • edited Loading

snewcomer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cw789 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cw789 Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismccord commented Jun 22, 2021

gcauchon commented Jun 22, 2021

bcardarella commented Jun 22, 2021

bcardarella commented Jun 22, 2021

bcardarella commented May 29, 2021 •

edited by mcrumm

Loading

tomciopp commented Jun 8, 2021 •

edited

Loading

bcardarella commented Jun 8, 2021 •

edited

Loading

snewcomer left a comment •

edited

Loading

cw789 Jun 16, 2021 •

edited

Loading