Update 0.12 - Githubissues

purescript-contrib / pulp

A build tool for PureScript projects

GNU Lesser General Public License v3.0

445 stars 86 forks source link

Update 0.12 #362

Closed hdgarrood closed 5 years ago

hdgarrood commented 5 years ago

Following on from #360

hdgarrood commented 5 years ago

@nwolverson do you think you might be able to take a look at this? there are just two tests which are failing, which are related to browserifying with source maps. I first suspected that the change to Node.Path.resolve (which previously returned a String, now returns an Effect String) was the culprit, but I tried replacing it with a version which just returned a String, and no dice. Now I have no clue what could have caused this to stop working.

nwolverson commented 5 years ago

Has the browserify version changed at all? I remember the path resolution stuff being a nightmare to get right in all scenarios. I'll take a look when I can but honestly if it's not a trivial change I'd be minded to delete the source-maps for browserify support.

hdgarrood commented 5 years ago

Fair enough. I did recently update the versions of both browserify and sorcery, but the CI is passing in master (which are using those same updated versions).

I've had a new thought though: perhaps this is related to https://github.com/purescript/purescript/pull/3314?

hdgarrood commented 5 years ago

I've actually been thinking more recently that I'm not sure it makes sense to have a dedicated browserify command. The important difference between browserify and build is that the former pulls in npm dependencies, right? If that's all there is to it, I'd be tempted to do the following:

add a new option to build which specifies whether or not to include npm dependencies in the bundle,
deprecate browserify in favour of this new option,
not make it part of the public API which tool we're actually using to do the bundling (perhaps that would give us an opportunity to consider alternatives such as Parcel?)

If someone knows they want browserify specifically, I think it's safe to assume that they're capable enough to set it up themselves, separately from Pulp.

hdgarrood commented 5 years ago

Ok, the same problem does exist in pulp 12.3.1, it isn't a result of this PR. It must be to do with something that changed in the compiler in the 0.12.0 release; I'm betting it is indeed https://github.com/purescript/purescript/pull/3314. The reason we're only just seeing this in CI is that we were previously only testing against purs 0.11.x, and now we're testing against purs 0.12.x instead. I'm going to just mark the failing tests as pending for now so I can get this merged.

hdgarrood commented 5 years ago

Eventually we should provide a mechanism for running tests against more than one compiler version (although the test suite is so slow as it is, I am slightly nervous about how long it would take to run the whole test suite twice...)

nwolverson commented 5 years ago

I'll try and take a look sometime, good to have the cause narrowed down.