tape-testing / tape

tap-producing test harness for node and browsers
MIT License
5.77k stars 307 forks source link

Automatically exclude node-specific packages from browser bundlers #564

Closed fregante closed 2 years ago

fregante commented 2 years ago

As discussed in https://github.com/substack/tape/issues/561#issuecomment-909470428

I'm opening a specific issue to this because the other one was due to a now-resolved Parcel issue.

In short: It'd be great to automatically offer a browser-only path to ensure:

ljharb commented 2 years ago

I'm not sure what you're asking for here.

The only correct thing for a node module bundler to do is provide browser polyfills for node core modules (when feasible), including per-module "globals". path and stream both work fine in the browser, so it's perfectly fine for tape (or any package) to depend on them unconditionally.

fs, of course, can't work in the browser, so it should be possible for fs to be substituted with an empty object, and the code should still work - which is what browserify and pre-broken webpack versions have always done. #565 may make it slightly easier for bundlers to handle fs, and that's great.

Webpack 5's choice means that every single user of it can't avoid extensive configuration, and it's not up to individual package authors to fix that problem - it's up to webpack's authors.

fregante commented 2 years ago

I'm not sure what you're asking for here.

To provide browser-specific entry points that don't make use of Node-specific tools.

it's perfectly fine for tape (or any package) to depend on them unconditionally

Yes and no. While they work in the browser, they still need to be bundled (even with browserify), increasing the bundle size (and in this case, configuration).

However, in tape’s case:

This means:

One possible improvement would be to exclude path somehow since it's currently only used here (in the non-bin entry point):

https://github.com/substack/tape/blob/a7ca7a308269bc3a250170441553d0321e0d8044/lib/test.js#L317

But it's not a big deal, so I'll close this issue.

ljharb commented 2 years ago

Bundling is a requirement of modern web dev; it's not practical to avoid using one. Bundle size shouldn't matter for a testing tool, even when run in a browser.

path.sep handles Windows cleanly, so i'm not sure how to exclude path there.

fregante commented 2 years ago

Bundling is a requirement of modern web dev; it's not practical to avoid using one

Where did I say otherwise?


Bundle size shouldn't matter for a testing tool

I said that 👇

  • bundle size is not a priority, since it's run locally

path.sep handles Windows cleanly, so i'm not sure how to exclude path there.

If you map path to false like we did for fs, you can then detect the lack of it and just avoid using it the whole code block. There are no paths in the browser (but I may be wrong, I'm not entirely sure about that file is doing) or probably it deals with URLs, why only have one separator.

But then again:

  • configuration for webpack 5 is still needed

So

it's not a big deal,

ljharb commented 2 years ago

ah, i misunderstood "they still need to be bundled", my bad.

it's not running locally that makes bundle size irrelevant, it's that "it's not for production, it's for dev". Bundle size only matters for production web code.

that's a good point. if __dirname and/or path are not present, we could skip https://github.com/substack/tape/blob/a7ca7a308269bc3a250170441553d0321e0d8044/lib/test.js#L366, and if __dirname is not present (ie, in broken bundlers), we could skip that entire logic.

I'd be willing to accept a PR for that, since the only cost paid by those using working bundlers is a pretty trivial if check.

fregante commented 2 years ago

I'd be willing to accept a PR for that

Thanks! However this would need some testing to ensure it works correctly and it's probably not worth the effort at this point, especially since I'm not entirely familiar with that piece of code.