Migrate client-admin from gulp to webpack

[patcon]

Addresses https://github.com/compdemocracy/polis/issues/90

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

Current status:

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

[patcon]

This should be good for review whenever you get a sec, @colinmegill 🙌

[patcon]

Any feedback on this @colinmegill? Or even just a sense of your priorities, and when it might have a chance to be reviewed. Thanks!

[patcon]

Bumping this. Any feedback?

Would love to have this reviewed, as the approach can be copied into other places, replacing gulp completely (which has been holding us back from upgrading and aligning node versions across repo)

I was waiting for feedback on approach before spending time on the final sweep of documentation, but let me know if you have other preference :) can also just merge and I'll update docs in another PR

[colinmegill]

Very in favor of direction, haven't had a chance to review in depth yet! Hope to soon

[patcon]

Pushed a final commit for README, so good to merge after review/changes :)

[patcon]

Friendly bump 🙌

[brendanarnold]

Big fan of this change - is a great step towards separating out build, deploy and run steps and provides a clear template for moving the participation and report bits of client code. It appear to be waiting on @micahstubbs Is there anything I can do here to help move this along? @micahstubbs @patcon @colinmegill @metasoarous?

[metasoarous]

Thanks so much for reviewing this @brendanarnold! We're big fans of this change too as it will help us remove a bunch of old tooling and dependencies preventing us from updating the environment, so very glad to have some eyes on it.

Were you able to get this working locally?

Given how big this PR is, I think we'd like to get in another review before merging.

Thanks again

[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.

[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:

• [x] Get up to date with edge (looks like conflicts should be minimal)
• [x] Build targets should go to build instead of dist, as discussed (https://github.com/compdemocracy/polis/pull/1242#discussion_r991718059)
• [x] Update file-server/Dockerfile to use build:prod instead of deploy:prod (https://github.com/compdemocracy/polis/pull/1242#discussion_r900490190)
• [x] Switch to inject: body (https://github.com/compdemocracy/polis/pull/1242#discussion_r776461657)

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 :)