search

pinojs / pino-webpack-plugin

MIT License
19 stars 9 forks source link

Support bundling on windows #134

Closed madtisa closed 12 months ago

madtisa commented 1 year ago

Current implementation of the plugin generates invalid paths on windows:

globalThis.__bundlerPathsOverrides = {'pino/file': pinoWebpackAbsolutePath('.\path.js')...

instead of escaped paths:

globalThis.__bundlerPathsOverrides = {'pino/file': pinoWebpackAbsolutePath('.\\path.js')...

This PR should fix this problem. (similar to #102)

simoneb commented 1 year ago

@madtisa tests don't pass

madtisa commented 1 year ago

@madtisa tests don't pass

Fixed, should pass now

madtisa commented 1 year ago

Here is info regarding the error on node 14 (win): https://github.com/tapjs/tapjs/issues/843. Looks like the only workaround they've found is to update npm. Alternatively, we can downgrade tapjs to 16.3.0 or drop node 14 support. What do you think?

mcollina commented 1 year ago

Please exclude Windows & Node.js v14 combo on GHA. It's broken everywhere.

simoneb commented 1 year ago

@madtisa as per Matteo's comment, feel free to remove Node 14 from the build matrix in this same PR

madtisa commented 1 year ago

done

simoneb commented 1 year ago

Looks like 16 is failing too now?

madtisa commented 12 months ago

Looks like 16 is failing too now?

Tested it locally on debian (wsl) with node 16 and tests pass, so I have no idea why it fails here (especially when they pass before I only disabled testing on node 14). I'll remove testing on v16 as you suggested, but I gradually losing faith in these tests. Btw, I could add testing on node v20 to compensate for it.

I would also suggest to include package-lock file to make builds more predictable (and reproducible).

simoneb commented 12 months ago

Thanks for all the work @madtisa , I'm going to merge this now