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

Demo: client-admin using razzle with customizations #515

Closed
wants to merge 55 commits into from

Conversation

patcon
Copy link
Contributor

@patcon patcon commented Aug 10, 2020

Re-ticketed from #514

The polis apps have quite a bit of special boilerplate and unique ways of doing things. I was curious whether a framework could help us hide some of this complexity and make updates more straightforward and along a clear path. (e.g., we get new dev standards without thinking too hard)

Razzle is an extension of create-react-app dev framework originally intended to support server-side rendered (SSR) apps. But relevant to us, it has added a mode that reverts back to single-page app (SPA). Razzle's SPA mode effectively offers the developer CRA with ability to customize webpack, which create-react-app doesn't have.

About Razzle

✅ Last commit: 3 days ago
👪 Contributors: 132
⭐ Stars: 9.7k
🔨 Used by: 1,770 projects

Screen Shot 2020-11-14 at 8 02 51 PM

To Do

  • gzip compression
  • geneate headersJson
  • remove deploy scripts
  • remove webpack config
  • move polis.config.js into .env
  • get docker-compose build working
  • remove dev-server.js
  • clean extraneous package inclusions from package.json
  • fix globals usage
  • webpack deploy to s3
    • test
  • add bundle analyzer npm script
  • webpack deploy via scp [?] wontdo Demo: client-admin using razzle with customizations #515 (comment)
  • remove gulpfile

@patcon patcon moved this from To do to In progress in Polis development Aug 12, 2020
@patcon
Copy link
Contributor Author

patcon commented Aug 12, 2020

Ditching SCP deploy, since @metasoarous says we're not using it:

We are not using scp deploy, just s3. I believe we added scp because that seemed like the most general second option we could support for pushing to static file servers. However, with the docker setup, that may now be obsolete, so I'm okay with removing if it cleans up some things. [...]

@patcon patcon changed the title Demo: client-admin using create-react-app/razzle with customizations [skip ci] Demo: client-admin using create-react-app/razzle with customizations Aug 12, 2020
@patcon
Copy link
Contributor Author

patcon commented Aug 26, 2020

@ballPointPenguin and I had a nice call to discuss the premise of using Razzle. He wasn't initially a fan, but became a convert as we discussed. Here's what we came to:

Risks

  1. razzle.config.js is another layer of indirection from webpack.
  2. Relying on another FOSS project with uncertain maintenance future.
  3. Razzle might alienate newcomers who are unfamiliar.

Mitigations & Benefits

  1. razzle.config.js is pretty easily recognizable as derived from webpack config.
  2. Quite widely used and active development (~10k ⭐, 122 contributors)
  3. Razzle offers great docs.
  4. Straightforward to leave Razzle if required: Just re-implement webpack config as we'd be doing now anyhow.
  5. Allows us to remove much of our custom scripts and gulpfile.
  6. Familiar behavior between razzle vs react-scripts as CLI.
  7. Replaces many dev packages with a single meta-package that upgrades in sync.
  8. Length of razzle.config.js is a rough measure of how many "nonstandard" things we're doing, so can think of minimizing it's size as "progress toward simplicity".

cc: @colinmegill

@patcon patcon changed the title Demo: client-admin using create-react-app/razzle with customizations Demo: client-admin using razzle with customizations Aug 26, 2020
Comment on lines +1 to +8
export const domainWhitelist = [
'^localhost$',
'^192\\.168\\.1\\.140$',
'^pol\\.is',
'.+\\.pol\\.is$',
'^xip\\.io$',
'.+\\.xip\\.io$'
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will need a rethink when we start preparing this setup for generalized production.

I think we're safe to defer this for now, since it works on localhost, pol.is domains, and xip.io development, which is really all it's "sanctioned" for rn

@patcon
Copy link
Contributor Author

patcon commented Nov 15, 2020

Just want to clarify that this Razzle approach is the common way to use CRA as boilerplate meta-package, but customize webpack. react-app-rewired, which perhaps seems more straight-forward a choice of tool at first blush, actually recommends using Razzle in SPA mode:
https://github.com/timarney/react-app-rewired#rewire-your-app-

would really love to try to discuss merging (both myself and @ballPointPenguin have come to conclusion that this approach is suitable and very easy to back out of if we don't like it), and would make all our special boilerplate go away asap. cc @colinmegill

@patcon
Copy link
Contributor Author

patcon commented Nov 15, 2020

Added details to issue body that I think bolster that this is safe (and again, very low cost to back out of -- just do the work to customize reactjs manually, by ejecting CRA or something. Or alternatively (ideally after trying Razzle for a stint 🤞 ), can migrate our razzle.config.js to craco.config.js (which is another CRA rewire tool):
https://github.com/gsoft-inc/craco

It has a higher project "Used by" count (5,000 vs 1,700 for razzle), but less stars (2.5k vs 10k) and contributors (30 vs 130)

@patcon patcon moved this from In progress to Feedback Required in Polis development Nov 15, 2020
@metasoarous metasoarous removed this from Feedback Required in Polis development Feb 25, 2021
@metasoarous
Copy link
Member

I still don't entirely understand what problem this solves. As far as I can tell, the potential benefits (paired down a bit from @patcon's list above, which is heavy on "mitigation" and marketing) are:

  • Allows us to remove much of our custom scripts and gulpfile.
  • Replaces many dev packages with a single meta-package that upgrades in sync.

We definitely need to get off gulp, but it seems like there are other ways to do that. I'm also somewhat ambivalent about "many dev packages" vs "single meta-package". There are typically tradeoffs both ways on that axis.

I think the main (implicit) argument for doing something like this would be if it reduced the amount of configuration and maintenance needed for the "many dev packages". As far as I can tell though, this PR doesn't seem to accomplish that. It looks like (as it stands) this PR would add nearly 16k lines and remove only 9k.

image

Am I missing something? Does this PR not yet reflect the full scope of benefits we'd get from it as far as simplifying the code base?

IIRC @colinmegill seemed apposed to this (I think on the grounds that Razzle was built for something different (SSR), and may be too opinionated for our needs), but @patcon wanted to do a POC to try to convince us it was a good idea. I'm curious to hear more of @ballPointPenguin's take, as well as from @micahstubbs regarding whether this would be indeed be helpful for the TS conversion (as was suggested in #960).

I do appreciate all the work that has gone into this, but at present am pretty skeptical that this is a good fit.

Thanks

@patcon
Copy link
Contributor Author

patcon commented Sep 5, 2021

IIRC @colinmegill seemed apposed to this (I think on the grounds that Razzle was built for something different (SSR), and may be too opinionated for our needs)

I recall it being that CRA was seen as more suitable to "toy web apps", not a production app like polis.

It looks like (as it stands) this PR would add nearly 16k lines and remove only 9k.

Vast majority of that is lockfile metadata. I believe all other files touched either disappear, get smaller, or stay the same. Compare dev branch's root dir (32 files) with create-react-app branch's root dir (16 files)

Def eager to hear @ballPointPenguin chime in on his perspective, but also eager to see this tossed out, and let someone else take a go. If that person experiences the same frustrations that led me to create this (e.g., recreating lots of CRA's webpack config and worrying about the maintenance burden, etc), then there'll be two people advocating for such an approach instead of just one.

Again, 171 LOC in razzle.config.js (a webpack analog) is what the crux of this is (replacing 508 LOC in gulpfile), with minimal cost to walk it back and remove razzle if someone later feels like hand-assembling the webpack config later.

@colinmegill
Copy link
Member

minimal cost to walk it back and remove razzle if someone later feels like hand-assembling the webpack config later.

If this PR was a migration from gulp to webpack, as you suggest here, I think it would be an easy decision to merge

@patcon
Copy link
Contributor Author

patcon commented Sep 5, 2021

Someone could probably recreate this with CRACO so as to be rewiring changes into CRA directly instead of using razzle (which natively allows rewiring). I opted for razzle because the maintainer of the leading "rewiring" tool (react-app-rewired: 8.4k stars) seemed to recommend it:

Note: I personally use next.js or Razzle which both support custom Webpack out of the box. -- timarney

@colinmegill
Copy link
Member

Ok, @patcon if you want to do the minimal work you describe here to make this webpack instead of razzle, I'll do a review. Let me know.

@patcon
Copy link
Contributor Author

patcon commented Sep 5, 2021

Thanks all, but my sense is that this PR isn't the right approach for this project, so I'm just going to pull the band-aid and close this, so something else can be started. If this is premature, please do re-open, but I feel like I've just drawn this out 🙃

@patcon patcon closed this Sep 5, 2021
@colinmegill colinmegill deleted the create-react-app branch September 5, 2021 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 chore ⚒️ infrastructure Re: automation, continuous integration. 🔩 p:client-admin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants