Migrate client-admin from gulp to webpack

[patcon]

Addresses #90

This is a work-in-progress, but could use some feedback on approach :)

Current status:

• removed gulp
• replaced gulp with webpack build
• since gulp gone, upgraded Node from v11.15.0 to v14.14.0
• npm run build:dev exists for unminified/uncompressed builds into dist/
• npm run build:prod exists for minified/compressed builds (as required for file-server)
• bundle analysis possible with npm run analyze
• removed extraneous files
• removed deploy for s3 (I can bring this back, as I know how to do it with webpack from Demo: client-admin using razzle with customizations #515)
• figure out what's wrong with e2e test on CI
  • all admin tests were failing
  • 2 embed/integration tests failing
• Figure out why bundlesize jumped from 175 kb => 210 kb ish
• local dev server working with webpack-dev-server
• Update README

The tests should pass with this 🎉 🤞

Open Questions

1. Is it alright to abandon S3 deploy code from gulp? My understanding is that production deploy is now done with docker-compose, right?

EDIT: I have feature parity for "webpack-based s3 deploy" code already written, so it's just a matter of copy-pasting it in here if we want to keep it

[patcon]

Hm. The admin interface is working fine when I do a docker build locally, but failing when run in CI. Not sure what's up.

[brendanarnold]

Thanks @metasoarous I havn't actually run this code yet - I'm working on getting a fork of pol.is up and running and I have the participant and admin code compiling and running OK so I can try with these changes in - I'll let you know how I get on.

[pmc30]

I did a test merge of this into the compdemocracy/dev branch, but I ran into this error when trying build with docker compose due to the removal of deploy:prod:

Step 11/42 : RUN npm run deploy:prod
 ---> Running in e2442f4e8746
npm ERR! missing script: deploy:prod
npm ERR! 
npm ERR! Did you mean this?
npm ERR!     build:prod

I see that client-admin/Dockerfile has been updated to use build:prod instead of deploy:prod, but it looks like when I am building it must be running either Dockerfile or file-server/Dockerfile, which both use deploy:prod and still reference node:11.15.0-alpine.

From looking at the docker-compose.yml file I think that it might be the file-server/Dockerfile that is making client-admin instead of client-admin/Dockerfile. I might just be doing something wrong when I build it, but it still might be good to remove gulp from the sections of Dockerfile and file-server/Dockerfile that relate to building client-admin so that they are all consistent

[metasoarous]

The problem is that there is some duplication of efforts in the client build process, as you note, between the file-server/Dockerfile and the individual client Dockerfiles. We duplicated this work into the client-server/Dockerfile to avoid issues with Dockerfiles referencing each other in certain build environments (Heroku, podman). I think we want to try and consolidate these if possible, potentially via multi-stage builds, but we shouldn't hang this PR up on that. We should however continue to keep all of the Dockerfiles up to date, and switch everything over to build:prod (vs deploy:prod). So, thanks for catching this.

[metasoarous]

@patcon Let's please keep the app/build path. I realize the way things were set up was kind of weird in that by default things were going to dist, unless you specified build in the LOCAL_OUPUT_PATH, but we should stick with the ultimate paths which were in use at the Docker level (app/build).

[patcon]

Sounds good, should be some small changes in webpack config and maybe places like gitignore (sorry, forget lots of what's done here atm)

[metasoarous]

Hey @patcon. Are you still interested in pushing this PR forward? Getting rid of gulp and upgrading node environment remains high priority for us, and there are an increasing number of issues blocked on it, so I want to try to get this resolved.

From what I can tell, here's what we need to merge:

• Right now, the PR is out of date, and needs to be rebased or merged onto edge.
• We'd like to go straight to Node 18 if possible; Node 14 leaves us with duplicate review work down the road.
• Ideally, apply the same treatment to the report client.
• Get output paths in line with what has previously been expected.
• Generally respond to any of the line comments above which need to be addressed.

I'm also curious why you moved a number of files into a public folder. Unless there is a good reason for this, please also revert these changes.

Thanks again

[patcon]

I'm open to this! Thanks for the poke @metasoarous

We'd like to go straight to Node 18 if possible; Node 14 leaves us with duplicate review work down the road.

That's likely quite a lot of further work, as many more deps will need to be tweaked, which will probably require other changes to codebase and maybe even substituted packages, and we'll probably want more tests for that. I worry I won't have time for the level of work that will entail, as it will be much more than just resolving a few merge conflicts imho

I could offer maybe 1-2h of exploration, but if it takes more than that, I'd ask that we merge without (or someone else runs with this PR).

If it's not simple, might you consider merging this smaller v14 node bump, just enough to use webpack, and push the other major upgrade into a separate upgrade cascade?

I'm also curious why you moved a number of files into a public folder. Unless there is a good reason for this, please also revert these changes.

Context: public is a standard known place in react projects (and many other JS frameworks) in which to keep files that are not built into the react app, but instead served statically, usually just copied into build dir. Nothing from that dir will be compiled by webpack, so it signals to developers that "hey, no need to look in here. these are static files that webpack doesn't build from". It's a nice assurance that is lost when webpack doesn't have that place. I suspect it also means exceptions must be added to keep content hashes from being added.

Might you reconsider a request to let it in? I believe it adds value, is a widely-understood convention, and not using it would make things slightly more confusing and explicit. I suspect if you did a straw poll with react devs, they'd all recommend using a public/ dir (and i suppose that as something easily done with an @mention here or quick twitter msg 🙂 ).

[patcon]

Ideally, apply the same treatment to the report client.

I'd request that this be another PR, as a matter of respect to the time I've already spent 🙏

[metasoarous]

Thanks for getting back on this @patcon. All fair points.

If you could then please:

• Get up to date with edge (looks like conflicts should be minimal)
• Build targets should go to build instead of dist, as discussed (Migrate client-admin from gulp to webpack #1242 (comment))
• Update file-server/Dockerfile to use build:prod instead of deploy:prod (Migrate client-admin from gulp to webpack #1242 (comment))
• Switch to inject: body (Migrate client-admin from gulp to webpack #1242 (comment))

Thanks again!

[patcon]

Woooo excited to find time for this. Thanks man

[metasoarous]

Hey @patcon. Any updates on this?

We have a number of other issues blocked on it, so I'd like to get a sense of whether you're still keen to push through to completion. If you're not able to right now, that's fine; I can pick it up from here, but wanted to give you the opportunity to wrap it up first.

Please let me know either way when you get a chance.

Thanks!

[patcon]

Hey Chris, sorry, been busy with some event stuff. I'm going to give myself until this weekend, after which I should officially release this back to you and stop blocking. Would that work?

[metasoarous]

Hey Pat. That's fine. Thanks for letting me know.

[patcon]

This is so rad! Thanks so much for digging this out of my 80%-hole @metasoarous :)

[metasoarous]

We actually needed this part of the routine for caching to work properly

[brendanarnold]

Webpack takes care of 'cache busting' Javascript import for you - in webpack.config.js line 32 it uses a 'magic' string [chunkhash:8] to generate a random 8 character string and puts that in the output filename. In the HtmlWebpackPlugin it is injected into the HTML body when it is processed - some earlier discussion into this is at https://github.com/compdemocracy/polis/pull/1242/files#r776461657

[metasoarous]

I don't know if we needed this or not

[brendanarnold]

This value is written but never read in the codebase so presumably it is a human readable version. There is some argument to be said that maybe this should be displayed visually so we know which version of the front-end we are looking at. In polis-whitelabel I actually mapped this to the package.json version with the view of later setting up formal releases.

[metasoarous]

That's what I thought, but thanks for doing some legwork to confirm!

We've been thinking about displaying version string as you suggest, and are interested in any ideas about it you might have.