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

feat(vite): add plugin to remove modules from the bundle #8973

Merged
merged 9 commits into from
Jul 28, 2023

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Jul 26, 2023

Analyzing the bundle produced by yarn rw build web with Vite reveals a few things we could improve. This PR improves one of them, which is not bundling the splash page you see when you have no Routes in dev. This sheds ~4% off a default Redwood app:

image

Next would be core-js-pure.

Note that I actually can't get this test to run. When I run yarn test in packages/vite, I get the following error...

ℹ ~/projects/redwood/redwood/packages/vite/src/plugins/__tests__/remove-from-bundle.test.mts:1
ℹ import { getShouldExclude } from '../vite-plugin-remove-from-bundle.ts'
ℹ          ^
ℹ 
ℹ SyntaxError: The requested module '../vite-plugin-remove-from-bundle.ts' does not provide an export named 'getShouldExclude'
ℹ     at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
ℹ     at async ModuleJob.run (node:internal/modules/esm/module_job:190:5)
ℹ 
ℹ Node.js v18.16.1
✖ ~/projects/redwood/redwood/packages/vite/src/plugins/__tests__/remove-from-bundle.test.mts (104.293542ms)
  'test failed'

@Tobbe was hoping you had ideas since I think you wrote the test in this package? I need to merge this, so not going to wait for that to be resolved. Will confirm it works manually for now, but if you get the chance, you can tell me or open a PR

Got them working.

@jtoar jtoar added the release:fix This PR is a fix label Jul 26, 2023
@jtoar jtoar added this to the v6.0.0 milestone Jul 26, 2023
@jtoar
Copy link
Contributor Author

jtoar commented Jul 26, 2023

This is rightfully failing CI—can't get the contact page to render at all when serving, and get an error in the console about the request splash-page returning the wrong mime type

@dac09
Copy link
Collaborator

dac09 commented Jul 27, 2023

and get an error in the console about the request splash-page returning the wrong mime type

Yeah I guess making it external isn't good enough, we need to make it load "nothing" when building.

Let me try another approach!

@dac09
Copy link
Collaborator

dac09 commented Jul 27, 2023

@jtoar pushed up a new version, have a look please!

I don't think node:test has been configured correctly - but I'm not going to bite on this.. is there a reason to not use Jest like all the other packages? The strange way of importing is something to do with module resolution in tsconfig compiler settings, but my preference would've just been to use jest.

@Tobbe maybe you configured node:test mind taking a look what's going on?

@jtoar jtoar modified the milestones: v6.0.0, next-release Jul 27, 2023
@jtoar
Copy link
Contributor Author

jtoar commented Jul 27, 2023

Thanks @dac09. I think we played around with node's test runner just to see what it would be like, since if we moved our packages to ESM Jest wouldn't understand how to test those files without Babel, or without other experimental Jest config.

Now that I've got some experience with it, node's test runner doesn't seem that much better than Jest w/ Babel. At least not if our TS tooling still thinks a package isn't ESM.

Update: was about to merge but noticed something here @dac09: #8973 (comment).

name: 'remove-from-bundle',
apply: 'build', // <-- @MARK important
load: (id) => {
excludeOnMatch(modulesToExclude, id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to return here?

Copy link
Collaborator

@dac09 dac09 Jul 28, 2023

Choose a reason for hiding this comment

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

Yep yep! Must've broken it trying get it working for the test! Pushing up in a bit

@dac09 dac09 enabled auto-merge (squash) July 28, 2023 07:08
@dac09 dac09 merged commit cb31df9 into main Jul 28, 2023
28 checks passed
@dac09 dac09 deleted the ds-vite/remove-splash-page-from-bundle branch July 28, 2023 07:34
@jtoar jtoar modified the milestones: next-release, v6.1.0 Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants