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

[WIP] try bundling CRWA #8071

Closed
wants to merge 6 commits into from
Closed

[WIP] try bundling CRWA #8071

wants to merge 6 commits into from

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Apr 20, 2023

⚠️ WIP

Not 100% sure this works yet, but I think the gist of it is here.

This PR:

  • bundles the CRWA package
    • all dependencies are now devDependencies so that yarn doesn't try to install anything (it just needs the file) when a user runs yarn create redwood-app
  • removes Babel from the @redwoodjs/tui build process in favor of using esbuild to transform syntax
    • I deliberately chose not to bundle this package since it's not an entry point, but a library (it can be bundled later by an entry point)

You can visualize meta.json files generated by the build scripts using esbuild's bundle analyzer: https://esbuild.github.io/analyze/. Here's an example of create-redwood-app's bundle:

image

To further reduce bundle size, the flame chart above suggests we should look into finding an alternative for systeminformation.

@jtoar jtoar added the release:chore This PR is a chore (means nothing for users) label Apr 20, 2023
@replay-io
Copy link

replay-io bot commented Apr 20, 2023

16 replays were recorded for 7f7c7f5.

image 0 Failed
image 16 Passed
    requireAuth graphql checks
          ```
          locator.waitFor: Target closed
          =========================== logs ===========================
          waiting for locator('.rw-form-error-title').locator('text=You don\'t have permission to do that') to be visible
          ============================================================
          ```
        </ol>
      </details>
      <li><a href=https://app.replay.io/recording/063576f2-6dd8-4bc8-b9c5-56eefc4c4f1c>useAuth hook, auth redirects checks</a></li>
      <li><a href=https://app.replay.io/recording/547a1a7c-0272-4105-a11b-ed08a8c1d1ed>Check that a specific blog post is prerendered</a></li>
      <li><a href=https://app.replay.io/recording/ba866bc2-bcdf-4c10-b2e1-dc94226175f3>Check that about is prerendered</a></li>
      <li><a href=https://app.replay.io/recording/a64e75ce-cb5e-4286-9717-8ed3ddce3e37>Check that homepage is prerendered</a></li>
      <li><a href=https://app.replay.io/recording/a20fbc84-a39a-48f7-a014-3973221ad4a7>Check that meta-tags are rendering the correct dynamic data</a></li>
      <li><a href=https://app.replay.io/recording/a5e1863c-e9cb-4440-a381-2cd5b4ac1878>Check that you can navigate from home page to specific blog post</a></li>
      <li><a href=https://app.replay.io/recording/4bd376b5-6f55-46f5-9086-2c50170ae4f8>Waterfall prerendering (nested cells)</a></li>
      <li><a href=https://app.replay.io/recording/7ad5043b-9c02-450e-8a16-6cfb2bf15ad6>RBAC: Admin user should be able to delete contacts</a></li>
      <li><a href=https://app.replay.io/recording/acb0ad84-3fe5-4f2c-a14b-ef83c207df56>RBAC: Should not be able to delete contact as non-admin user</a></li>
      <li><a href=https://app.replay.io/recording/2b568879-5410-4bf0-a0dc-884c197b8726>Smoke test with dev server</a></li>
      <li><a href=https://app.replay.io/recording/d5cdb976-5ae2-4fee-895a-55d9be9ea019>Smoke test with rw serve</a></li>
      <li><a href=https://app.replay.io/recording/2b94459a-9fe1-46b2-866f-fc4b91bf5455>Loads Cell mocks when Cell is nested in another story</a></li>
      <li><a href=https://app.replay.io/recording/dd955ca0-648e-4a3b-bd56-e483b1ddf727>Loads Cell Stories</a></li>
      <li><a href=https://app.replay.io/recording/158338d6-12af-46ca-a0bb-6a0dbcb7e240>Loads MDX Stories</a></li>
      <li><a href=https://app.replay.io/recording/005e34f0-15a8-4dc0-85e5-da41d9bfc6c3>Mocks current user, and updates UI while dev server is running</a></li>
      

View test run on Replay ↗︎

@Josh-Walker-GM
Copy link
Collaborator

Josh-Walker-GM commented Apr 21, 2023

I tested the results of this locally and all the various crwa options worked as expected. I also tested with with the minify: true option and things were fine in that case too. The resulting size decrease from minification was:

image

On the issue of potentially removing systeminformation then it seems to boil down to logical vs physical core count. Node's os package can return the total system memory just fine but it returns the number of logical cpus as opposed to the number we currently get from the systeminformation package which is the number of physical cpus. Maybe we're fine with the logical number or perhaps we just keep using the systeminformation package to track the same value.

At least my testing locally showed this, perhaps os won't return the expected values in situations like inside a vm. Will need a further test of that.

(If that bundled+minified size is equivalent to the installed package size reported for the current stable create-redwood-app this would be a 99.2% reduction in size)

@jtoar
Copy link
Contributor Author

jtoar commented Apr 21, 2023

@Josh-Walker-GM great finds! Leaving some more notes here. The main thing to watch out for in bundling node_modules is this (source: https://esbuild.github.io/getting-started/#bundling-for-node):

You also may not want to bundle your dependencies with esbuild. There are many node-specific features that esbuild doesn't support while bundling such as __dirname, import.meta.url, fs.readFileSync, and *.node native binary modules. You can exclude all of your dependencies from the bundle by setting packages to external:

Disabling minify and doing a quick search for __dirname in dist/create-redwood-app.js brings up nine instances. Most of them are yargs, but one of them is ours. That one is ok because the directory structure is maintained, but the yargs cases are most likely broken. The question is if it matters (maybe those functions aren't executed). And that's just for __dirname. The fs module poses a similar concern.

Given this, maybe we should break this PR into two: 1) just remove Babel so we at least shed the dependencies on @babel/runtime-corejs3 and core-js. Then worry about bundling

@jtoar jtoar force-pushed the ds-crwa/try-bundling branch from e83b37d to f8e6002 Compare April 26, 2023 02:36
@jtoar jtoar closed this Dec 14, 2023
@jtoar jtoar deleted the ds-crwa/try-bundling branch December 14, 2023 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:chore This PR is a chore (means nothing for users)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants