TylorS / snowpack-plugin-hash

Apply content hashes to your production build
15 stars 2 forks source link

Files not in depedency tree will break the build #12

Open deviousdodo opened 3 years ago

deviousdodo commented 3 years ago

Having a file with jsx that is not imported anywhere in the app will result in this error when building:

UnhandledPromiseRejectionWarning: Error: Can't resolve '../_snowpack/pkg/react.js' in '/app/build/dist'

Here's an example file:

> cat src/standalone.tsx
import * as React from "react";

export function hello() {
  return <span>world</span>;
}

I think these files should simply be ignored by the plugin - this is what esbuild does as well.

The other problem is that the build command doesn't fail - so the CI is always green (not sure if this is due to the plugin or snowpack).

I've tried to fix the issue by myself, but couldn't figure out what's the best way to proceed. I could have another look if you can give me some pointers.

TylorS commented 3 years ago

Hey @deviousdodo thanks for reporting your issues. I honestly thought this was a covered use case as one of the steps is determining the dependency graph and ordering it.. which reveals all of those not imported.

I'll see what I can do to fix this situation

TylorS commented 3 years ago

Hey @deviousdodo I added this file as a test case to @typed/content-hash here and it would seem like things are working as I expected.

Would you be able to help me out with a reproduction or see if a newer version has fixed this issue?

deviousdodo commented 3 years ago

Thanks for looking into this! I tested again with latest snowpack & plugin version and it still happens, but I just realised it only happens when bundle is set to true in snowpack.config.

I created an example repository over here: https://github.com/deviousdodo/snowpack-plugin-hash-bug

The repo was initialized with npx create-snowpack-app repo --template @snowpack/app-template-react-typescript and on top I added src/Unused.tsx, installed the plugin and configured snowpack to bundle. Running npm run build in the repo will show the error.

Edit: I just read the deprecation issue you posted. It's probably not worth spending time on this bug in this case (we'll also drop the dependency if there will be a native solution).

TylorS commented 3 years ago

I'll see what I can do in the meantime to help your situation out...it helps make @typed/content-hash better in the long run. Thanks for the reproduction repo 🙏

Yeah, once snowpack allows setting up esbuild to do content hashing it will use a parallelized algorithm that node just can't handle too well and won't require traversing an AST of all of the outputs.

I dunno how invested you are in snowpack, but I had made the switch to Vite in a project at work when I was running into a bunch of bugs pre v3.1 (lots of improvements AFAICT) and it was was pretty painless and they support this out-of-the-box as well using rollup if content hashing w/ a bundle is important to you

deviousdodo commented 3 years ago

We're not that invested in snowpack, but I really like esbuild's performance and since this isn't really such a big issue we'll wait it out a bit more :)

We did run into a few bugs with Snowpack and I'll give Vite a try in the next project if we'll run into more.