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

Tree-shake JS using build result, instead of source #475

Closed
FredKSchott opened this issue Jun 11, 2020 · 2 comments · Fixed by #526
Closed

Tree-shake JS using build result, instead of source #475

FredKSchott opened this issue Jun 11, 2020 · 2 comments · Fixed by #526

Comments

@FredKSchott
Copy link
Owner

FredKSchott commented Jun 11, 2020

We're seeing more and more issues related to our tree-shaking scanner not working properly on source files, when the build changes imports:

  • TypeScript: import { SomeTypeOrInterface } runs into errors, since SomeTypeOrInterface doesn't exist as an actual JS export, only as a TS type. Our current workaround is to force users to type import type { SomeTypeOrInterface } instead, but this isn't ideal.
  • Emotion / @emotion/core: The babel plugin adds a new jsx import at build time, but this isn't detected when we scan the source file so it's removed via tree-shaking. (See: https://www.pika.dev/npm/snowpack/discuss/356)

During development, we're still limited to scanning source files so that we can deliver on our promise of a fast dev environment. If we built every file before scanning, that would get slower as your application grew. BUT... since our build command is by definition building the entire application, it feels like we should take advantage of this higher-fidelity result so that our tree-shaking algo can be as efficient (and as error-free) as possible.

Solution?

One thing that I just realized: we can run our installer AFTER a successful build, and scan the build result directory instead of your source directory. This should solve a whole host of issues around imports that are added/removed/changed during build, or that never exist at all (ex: Svelte core library is never tree-shaken, since no svelte/internal imports actually exist in your source code).

Ironically, this goes against something that I'd said in #441 - we would now need to continue to support scanning /web_modules/react.js, or /custom/web_modules_dir/react.js. #483 intentionally left this work TODO so that we could resolve here.

@FredKSchott FredKSchott changed the title Tree-shake JS in build result, instead of source Tree-shake JS using build result, instead of source Jun 11, 2020
@FredKSchott
Copy link
Owner Author

Okay, I took a stab at this and it's working well for the basic usecase. But, it will still require some work to be PR ready: https://github.com/pikapkg/snowpack/compare/wip-install-after-build?expand=1

The current workflow:

  • Scan your source code, and install your web dependencies to node_modules/.cache/snowpack/build/*
  • Run the build, which "mounts" the cached dependencies dir to the final build location

This workflow is what causes the issues outlined above: We tree-shake in production, but we're tree-shaking based on the EXACT imports of your source code which aren't always === the imports you actually need in production (example: TS type imports)

This WIP branch introduces a new workflow:

  • Scan your source code to generate only the web_modules import map (needed during build) without running a full install.
  • Run the build, which does nothing with your web_modules.
  • Scan your final build, and install your web dependencies directly to build/web_modules/*

We could even take this a step further:

  • No pre-build install step needed.
  • Run the build, but don't resolve imports yet. Leave import "react" in the code.
  • Scan your final build, and install your web dependencies directly to build/web_modules/*
  • Resolve imports now, using the import map generated in the previous step.

Surprisingly, the fact that we already split "building" and "import resolving" into two different steps in build/dev means that this isn't "one step further" proposal isn't as strange as it sounds. It actually removes an entire problem set related to "what do we do when we can't find an import in the import map" during build: the import map is generated from the final build, instead of the reverse that we have today.

@FredKSchott
Copy link
Owner Author

FredKSchott commented Jun 19, 2020

(https://github.com/pikapkg/snowpack/compare/wip-install-after-build has been updated)

Update: I actually kind of like the direction that the "one step further" proposal above is heading in:

  1. Source files are built (not yet written to disk, just built into memory).
  2. Built files are scanned for the exact set of dependencies needed in the final build.
  3. Installation runs for the exact set of dependencies & imports used in the final built app.
  4. Built file imports are resolved.
  5. Built files are written to disk.

This is nice in that there's very little duplicate work across steps: just 1 read from disk step, 1 build step, 1 scan step, 1 resolve step, and finally 1 write to disk step.

A small downside is that it requires a programatic interface to call install() with a set of install targets already scanned from memory (vs. the current default behavior that scans your project on disk). This is something we don't yet have, but that was already being ask for by others (latest in #518).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant