nwolverson / purescript-language-server

MIT License
183 stars 41 forks source link

Bundling broken #161

Closed nwolverson closed 2 years ago

nwolverson commented 2 years ago

Recent TS update has broken purs bundle again. This doesn't seem to work well with TS generated JS, possibly just use parcel here

wclr commented 2 years ago

I didn't check the build, as i'm using a direct version from the output when developing.

As a temporary workaround may just add an intermediate build script (between build:tsc and build:server) that will remove the malicious string from generated js output, to be cross platform using for example the package, script will be:

replace-in-files-cli --regex ".*void 0;" --replacement "" src/**/*.js

Temporary in terms that the compiler should anyway support ES module anytime soon (but who knows when).

The bundler will work too, but the build needs some changes in code and config and parcel is not very good at configuring (you can not even pass the output filename using cli). I would propose to use esbuild instead:

esbuild --bundle --platform=node --minify ./output/LanguageServer.IdePurescript.Main > bundle.js

bundle.js should then be imported in server.js and bundler will not add calling the main() entry point) It also supports watch mode and works superfast.

If you want I can make a PR (for any of the variants).

nwolverson commented 2 years ago

I guess you came to the same conclusion, that it is the re-assignment after the void 0 initialisation of the exports, which causes the last export (which was the 1st in that void 0 line) to be dropped.

The suggestion of parcel is because the CI currently uses that to make a single-file bundle with npm dependencies baked in.

I think I have a preference for esbuild over a regex replace, it would seem to be about as much work to implement and the regex approach

  1. Is hacky
  2. Retains a dependence on purs bundle which probably goes away at some time

Feel free to make a PR for the esbuild version, though there is no rush - local dev against output/ is fine anyway.

nwolverson commented 2 years ago

Fixed by #163