Undo removal of run.js #866

sigma-andex · 2022-03-11T18:30:30Z

Description of the change

Undo #846 since this does not work in the case of cli commands, see this comment.
Use run.mjs in the case of es modules since otherwise this will fail if "type":"module" is not set in the package.json.

Checklist:

Added the change to the "Unreleased" section of the changelog
Added some example of the new feature to the README
Added a test for the contribution (if applicable)

P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊

JordanMartinez

Just a few comments about this overall.

src/Spago/Build.hs

JordanMartinez · 2022-03-11T20:00:23Z

Also, by reverting back to using files for run/test, we need to reopen #822. I think that issue should be tackled in a separate PR because it also affects CJS modules, too.

sigma-andex · 2022-03-11T20:28:27Z

Also, by reverting back to using files for run/test, we need to reopen #822. I think that issue should be tackled in a separate PR because it also affects CJS modules, too.

yep makes sense. I wonder if there is a reason to use an absolute path, this problem shouldn’t happen with relative paths I guess

JordanMartinez · 2022-03-11T21:01:25Z

yep makes sense. I wonder if there is a reason to use an absolute path, this problem shouldn’t happen with relative paths I guess

I'm not sure absolute paths are needed, but it seemed like it might have been needed for pulp test to work.

f-f · 2022-03-18T10:02:29Z

Also, by reverting back to using files for run/test, we need to reopen #822. I think that issue should be tackled in a separate PR because it also affects CJS modules, too.

I wouldn't like to fix a bug by introducing a regression, so I think that bug should also be fixed in this PR.
To reproduce it we just need to have a project's folder name contain a space, so it's a test that we can easily add (and the fix should be very easy too I suspect)

sigma-andex · 2022-03-18T11:12:43Z

I wouldn't like to fix a bug by introducing a regression, so I think that bug should also be fixed in this PR.
To reproduce it we just need to have a project's folder name contain a space, so it's a test that we can easily add (and the fix should be very easy too I suspect)

Regarding tests: I think it would be also nice to have some tests for this, since it has grown now in logic. But the problem is that we would need different purescript versions and I am not sure how we would do that.
The alternative would be to refactor it and provide unit tests instead.. Do you have a proposal on how to provide some test for this whole bundling logic?

sigma-andex · 2022-03-20T13:23:34Z

I have added the escaping of the file name with spaces, which should fix the bug. I added also two more fixes for source maps and esm format on node when bundling modules.

As it is right now, it is ready for review, but I still don't know how to write some test cases without refactoring the whole thing. Also I don't really know how to add a test for the space in file path bug.

sigma-andex · 2022-03-21T15:39:13Z

Hi @f-f , do you have time to review this soon? It is currently blocking the v0.15.0 ecosystem update since the purescript-contrib libraries depend on spago test. Thank you! :-)

JordanMartinez

Just some general comments.

Also, I think a test can be added for #822 by changing this line to replace spago-test with spago test. However, when I did that locally, a few other tests failed, too.

CHANGELOG.md

src/Spago/Build.hs

thomashoneyman · 2022-03-21T17:20:09Z

Hi @f-f , do you have time to review this soon? It is currently blocking the v0.15.0 ecosystem update since the purescript-contrib libraries depend on spago test. Thank you! :-)

We can also comment out spago test temporarily in CI and only rely on spago build, switching to pulp test for the time being. After all, we need to add bower and pulp to all the contrib libraries too. That way @f-f can have some breathing room to review as he’s got a ton of things on his plate.

JordanMartinez · 2022-03-21T20:59:25Z

We can also comment out spago test temporarily in CI and only rely on spago build, switching to pulp test for the time being. After all, we need to add bower and pulp to all the contrib libraries too. That way @f-f can have some breathing room to review as he’s got a ton of things on his plate.

Works for me!

f-f · 2022-03-23T07:51:13Z

src/Spago/Cmd.hs

+import qualified Data.Versions  as Version
+
+-- | Get the semantic version of a command, e.g. purs --version
+getCmdVersion :: Text -> RIO env (Either Text Version.SemVer)


Having a whole separate module only for this is a bit noisy - we usually hold all these utilities in our custom Spago.Prelude, could we move this function there?

I did this before, but then I ran into a cyclic dependency issue, that's why I had to create a separate module

CHANGELOG.md

f-f · 2022-03-23T10:19:20Z

@sigma-andex a few more notes:

as Jordan noted, to reproduce the "space in path" bug we need to rename the test directory to have a space in it. Let's do that and see what else breaks
to test different purescript versions we could rework the build to be nix-based, as we're about to do in the Registry. AFAIK @thomashoneyman might have some input on this, as he's been looking at Update chameleon-transformers registry#360.
Alternatively, and in a less disruptive way, we could install purescript 0.14, run tests, then install purescript 0.15 and run tests again. We have some setup to deal with a similar things for the psa-related tests, as we run them both with psa and without.

JordanMartinez · 2022-03-23T13:58:18Z

Alternatively, and in a less disruptive way, we could install purescript 0.14, run tests, then install purescript 0.15 and run tests again. We have some setup to deal with a similar things for the psa-related tests, as we run them both with psa and without.

This sounds like the easiest way forward.

Removal of run.js interfered with cli programs. Generate run.mjs for projects >= 0.15 Relax es modules version constraint to 0.15.0-alpha-01

Co-authored-by: Fabrizio Ferrai <fabrizio.ferrai@gmail.com>

JordanMartinez · 2022-04-06T11:42:09Z

@sd-yip sorry I haven't seen your PR before. Looks good to me! @JordanMartinez any comments from your side or can we merge @sd-yip 's change ?

Other than 'thanks!', nope! 😄

This change forces the `build` command's `Env` arg to have more capabilities. For now, I've propagated that by granting it the `HasBuildEnv`, but that might be granting it too many capabilities.

- checkFixture and fixPackagesDhall works regardless of the number of nested dirs used - run and test tests run in another nested folder with space - run, test, bundle-app/module tests remain in original positions - run, test, bundle-app/module tests wrapped by a purs-0.15 label

f-f · 2022-04-14T13:54:20Z

Thanks @JordanMartinez for improving the diff! I'll have another look now 🙂

f-f

Looks great now! Thanks everyone 👏

I think this is good to merge once we figure out the NPM stuff.

JordanMartinez · 2022-04-26T20:38:00Z

Ok. CI builds! 🎉 @f-f Since you previously approved this and the last thing to resolve--the NPM installation--has now been resolved, I'm going to merge this.

JordanMartinez · 2022-04-26T20:46:38Z

Since the most recent commit gets rid of the metadata needed to install and runs purs via the tar download approach, CI now expects build (macOS-latest, macos, purs) and build (windows-latest, win64, purs.exe) builds to occur, which no longer exist. @f-f Not sure how that was changed last time, but just FYI.

f-f

Wonderful, we're good to go! Thanks everyone for contributing to this 😊

f-f · 2022-04-27T09:41:15Z

I'll cut a release later today so we get to use this ASAP

JordanMartinez suggested changes Mar 11, 2022

View reviewed changes

src/Spago/Build.hs Outdated Show resolved Hide resolved

src/Spago/Build.hs Outdated Show resolved Hide resolved

sigma-andex mentioned this pull request Mar 13, 2022

Command line arguments with spago run #865

Closed

JordanMartinez mentioned this pull request Mar 18, 2022

Update ecosystem to v0.15.x PS release purescript/purescript#4244

Closed

sigma-andex requested a review from JordanMartinez March 20, 2022 13:21

JordanMartinez suggested changes Mar 21, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/Spago/Build.hs Outdated Show resolved Hide resolved

src/Spago/Build.hs Outdated Show resolved Hide resolved

f-f reviewed Mar 23, 2022

View reviewed changes

sigma-andex and others added 13 commits March 23, 2022 21:17

Undo removal of run.js

f8cdd1c

Removal of run.js interfered with cli programs. Generate run.mjs for projects >= 0.15 Relax es modules version constraint to 0.15.0-alpha-01

Get node version and use experimental flag if necessary

9ef1f56

Remove bundle and src map test as the will fail on purs v0.15

94dfb5b

Remove nl from error message

4c6609e

Add package.json creation instead of run.mjs

7c84eeb

Fix format flag for node

4daa5b2

Fix sourcemap flag for esbuild

580c52a

Fix space in folder bug

1b1d7a0

Readd bundle test

afa1bae

Update changelog

9f79ff9

Fix pr comments

cff8842

Update CHANGELOG.md

137fe6c

Co-authored-by: Fabrizio Ferrai <fabrizio.ferrai@gmail.com>

Fix esm format flag

a990a4d

sd-yip added 2 commits April 6, 2022 06:41

Define runProcessWithOutput

0a6acaa

Supply stdin separately

f613890

JordanMartinez added 13 commits April 6, 2022 06:46

Drop fail-fast in build.yml

af6b034

Drop comment on packages.dhall prepare-0.15

222a743

Move Node code into nodeAction

2555923

This change forces the `build` command's `Env` arg to have more capabilities. For now, I've propagated that by granting it the `HasBuildEnv`, but that might be granting it too many capabilities.

Push things to top-level; use two test fns

6b4d25e

Actually dedent tests in noSpacesInParentDir

eca612e

Remove various formatting changes

1ed311a

Refactor setup' to reuse setup so one print cmd works

ac8d522

Refactor common cp code into cpFixture

a52a164

Minimize diff due to indentation

f8cd1a1

Document what purs-0.15 label means

b717ca7

Add empty blank lines deleted in PR

52d489c

Update usages of cp '../../fixtures/file' with cpFixture 'file'

73db627

f-f approved these changes Apr 14, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into undo-removing-runjs

d340cb8

JordanMartinez mentioned this pull request Apr 19, 2022

Prepare v6.0.0 release (1st PS 0.15.0-compatible release) purescript/purescript-prelude#292

Merged

6 tasks

Install purescript via next tag

df19289

Drop old metadata for previous purs installation

fc89ea1

thomashoneyman approved these changes Apr 26, 2022

View reviewed changes

f-f approved these changes Apr 27, 2022

View reviewed changes

f-f merged commit 678e004 into purescript:master Apr 27, 2022

sigma-andex deleted the undo-removing-runjs branch April 27, 2022 09:57

paulyoung mentioned this pull request Jun 3, 2022

spago test --no-install in checkPhase fails justinwoo/spago2nix#52

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Undo removal of run.js #866

Undo removal of run.js #866

sigma-andex commented Mar 11, 2022

JordanMartinez left a comment

JordanMartinez commented Mar 11, 2022

sigma-andex commented Mar 11, 2022

JordanMartinez commented Mar 11, 2022

f-f commented Mar 18, 2022

sigma-andex commented Mar 18, 2022

sigma-andex commented Mar 20, 2022

sigma-andex commented Mar 21, 2022

JordanMartinez left a comment

thomashoneyman commented Mar 21, 2022

JordanMartinez commented Mar 21, 2022

f-f Mar 23, 2022

sigma-andex Mar 23, 2022 •

edited

Loading

f-f commented Mar 23, 2022

JordanMartinez commented Mar 23, 2022

JordanMartinez commented Apr 6, 2022

f-f commented Apr 14, 2022

f-f left a comment

JordanMartinez commented Apr 26, 2022

JordanMartinez commented Apr 26, 2022

f-f left a comment

f-f commented Apr 27, 2022

Undo removal of run.js #866

Undo removal of run.js #866

Conversation

sigma-andex commented Mar 11, 2022

Description of the change

Checklist:

JordanMartinez left a comment

Choose a reason for hiding this comment

JordanMartinez commented Mar 11, 2022

sigma-andex commented Mar 11, 2022

JordanMartinez commented Mar 11, 2022

f-f commented Mar 18, 2022

sigma-andex commented Mar 18, 2022

sigma-andex commented Mar 20, 2022

sigma-andex commented Mar 21, 2022

JordanMartinez left a comment

Choose a reason for hiding this comment

thomashoneyman commented Mar 21, 2022

JordanMartinez commented Mar 21, 2022

f-f Mar 23, 2022

Choose a reason for hiding this comment

sigma-andex Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

f-f commented Mar 23, 2022

JordanMartinez commented Mar 23, 2022

JordanMartinez commented Apr 6, 2022

f-f commented Apr 14, 2022

f-f left a comment

Choose a reason for hiding this comment

JordanMartinez commented Apr 26, 2022

JordanMartinez commented Apr 26, 2022

f-f left a comment

Choose a reason for hiding this comment

f-f commented Apr 27, 2022

sigma-andex Mar 23, 2022 •

edited

Loading