I agree that having some tests would be good. Are you volunteering to add some? I'm very happy to help if you have any questions.

It sounds like morning fun if we do not mind me taking some time. An outline:

1. Example directory for purs files in tests/purs/bundle directory.
2. A module TestPscBundle.hs in /tests/ directory.
3. Similar setup as the compiler tests but using the psc-bundle.
   As an alternative, we could add an annotation 'use-bundle' to the compiler tests and keep the examples in there. I think it might be mixing concerns though.

I think a tests/purs/bundle directory sounds great.

(Fixes #3505)

@hdgarrood
Test added.
I have exported the common functions between compiler and bundle tests to TestUtils.
One problem I see is that in order to increase bundle coverage we would have to manually implement js scenarios. With the current solution (test vs compiled purescript code) - some code will be unreachable.
For now, it fixes the problem and adds coverage.
Please review.

One problem I see is that in order to increase bundle coverage we would have to manually implement js scenarios. With the current solution (test vs compiled purescript code) - some code will be unreachable.

This doesn't bother me, I think what we have here is fine for now.

Thanks for all the minor comments, they are applied.

I've now reviewed this properly and it looks good to me, although I have one small suggestion. Instead of e.g.

  var exports = PS["Effect.Console"] = PS["Effect.Console"] || {};

could we perhaps generate

  var exports = PS["Effect.Console"] || {};
  PS["Effect.Console"] = exports;

which I think should be equivalent, but is a bit clearer?

Oh and one more: how about using $PS as the name of the argument for the IIFE function for each module in the output? Then we should be able to avoid collisions with names from PureScript values entirely, rather than relying on slightly arcane details of JS scoping in cases of collisions.

All suggestions included in the PR.
I have changed the generated JS.
I have changed the default namespace to $PS (inside the bundle action in the app) as suggested.
It would not solve the issue by itself - a namespace provided to the bundler could still interfere with a constructor if they shared names.
Thus I have changed the name of the test and included a pragma to configure namespace provided to the bundler so that the test is still testing what it should (the case where the namespace is the same as the constructor).

Sorry, I should have been clearer: what I mean is that the default bundle namespace should continue to be PS, but the name given to the argument of each iife for each module should be something which couldn’t clash with any purescript name. So for example you might get

var PS = PS || {};
function($PS) {
  // Generated by purs version 0.12.2
  "use strict";
  var exports = $PS["Effect.Console"] = $PS["Effect.Console"] || {};
  var $foreign = $PS["Effect.Console"];
  var Data_Show = $PS["Data.Show"];
  var Data_Unit = $PS["Data.Unit"];
  var Effect = $PS["Effect"];
  exports["log"] = $foreign.log;
})(PS);

So the namespace object is always referred to as $PS inside each iife, regardless of the namespace CLI option. I’d prefer to leave the tests as they were before.

Oh, that sounds reasonable.
All changes are reverted/made.

The only difference to the example is in the namespace declaration.
Instead of the proposed

var PS = PS || {};

it is

var PS = {};

as it was before.
The semantics are different when a namespace is shared across bundles.
Which one should we go with?

Yes, thanks, I think the initial namespace declaration should stay as it is, var PS = {};. (I couldn't be bothered to check how it is currently and so I made a guess which obviously has turned out to be incorrect.)

Did you make a commit locally which you haven't pushed yet? It looks like there aren't any new commits to this branch since my last review comment.

I have rebased and pushed.
Everything is up to date.

The moduleExports declaration doesn't seem to have changed?

Thanks for noticing.
Somewhat I did not read the moduleExports comment.
Now it is addressed.

Thanks!