Add --source-maps flag

[Profpatsch]

> purs bundle --help
…
  --source-maps            Whether to generate source maps for the bundle
                           (requires --output).

> spago bundle-app --help
  --source-maps << why u no source map‽ 

It would be great if the --source-maps flag could be passed to purs. According to #216 (comment) it’s possible via purs bundle, but I gave up reverse engineering how exactly spago bundle-app calls purs (well, it doesn’t, it calls the library, which makes it harder even).

This should be easy to add.

[f-f]

@Profpatsch we don't use the library, but call the binary. The call to bundle happens here:

spago/src/Spago/Purs.hs

Lines 66 to 80 in cc75556

	bundle :: WithMain -> ModuleName -> TargetPath -> Spago ()
	bundle withMain (ModuleName moduleName) (TargetPath targetPath) = do
	let main = case withMain of
	WithMain -> " --main " <> moduleName
	WithoutMain -> ""
	cmd
	= "purs bundle \"output/*/*.js\""
	<> " -m " <> moduleName
	<> main
	<> " -o " <> targetPath
	runWithOutput cmd
	("Bundle succeeded and output file to " <> targetPath)
	"Bundle failed."

I'm up for adding a --source-maps flag to spago, and it would conditionally add a flag for purs as it happens in line 68.

However you can already pass things to purs today, this should work: spago bundle-app --purs-args "--source-maps"

[Profpatsch]

Sadly it doesn’t, because that option passes flags on to the toplevel of purs:

Invalid option `--source-maps'

Usage: purs COMMAND
  The PureScript compiler and tools

[Woody88]

Hello,

I really appreciate this library, so I can make time this weekend to work on it!

[Woody88]

Hey,

So, It implemented the feature. Everything seems to be working fine.
I added "--source-maps" flag, and also added, "-x" as the shorter version because "s" and "m" were already taken. If you prefer another letter please let me know.

I also added a test case:
https://github.com/Woody88/spago/blob/aa0e2ac550ab0885e04bd774867ab49ab325e19f/test/SpagoSpec.hs#L648-L653

However, line 653 is giving me this error:
1) Spago, spago bundle-app, Spago should bundle successfully with source map expected: "{\"mappings\":\";;;AAAA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;;;ACPA;AACA;AACA;AAsBA;;;ACxBA;AACA;AACA;AACA;AACA\",\"sources\":[\"output/Effect.Console/foreign.js\",\"output/Effect.Console/index.js\",\"output/Main/index.js\"],\"names\":[],\"version\":3,\"file\":\"/Users/woodson/Code/Haskell/spago/templates/bundle-app-src-map.js\"}" but got: "{\"mappings\":\";;;AAAA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;;;ACPA;AACA;AACA;AAsBA;;;ACxBA;AACA;AACA;AACA;AACA\",\"sources\":[\"output/Effect.Console/foreign.js\",\"output/Effect.Console/index.js\",\"output/Main/index.js\"],\"names\":[],\"version\":3,\"file\":\"/Users/woodson/Code/Haskell/spago/test/spago-test-1d8ce1c852162faa/bundle-app-src-map.js\"}"

I generated test/fixtures/bundle-app-src-map.js from the templates folder, so the file path is different from the one generated by the test. Should we only verify if the source-map file is generated instead?

[f-f]

I generated test/fixtures/bundle-app-src-map.js from the templates folder, so the file path is different from the one generated by the test. Should we only verify if the source-map file is generated instead?

@Woody88 yes, only checking that the file is there sounds sensible to me 🙂

[Profpatsch]

and also added, "-x" as the shorter version because "s" and "m" were already taken. If you prefer another letter please let me know.

Just leave it out, short forms are usually making scripts harder to read (especially if they don’t correspond to the first letter of the command), and I don’t think --source-maps is going to be used on the command line a lot.

[f-f]

Just leave it out, short forms are usually making scripts harder to read (especially if they don’t correspond to the first letter of the command), and I don’t think --source-maps is going to be used on the command line a lot.

We are using Turtle's wrapper of optparse-applicative, so actually having a shorthand is easier than not having it when patching the code. I'd like to eventually port the whole CLI parser to optparse-applicative since it got big enough, I'll open an issue for this.

[f-f]

Fixed in #562