Support upcoming v0.15.0 compiler#401
Conversation
|
Let's say const p = require("process");
console.log(p.argv);Using $ node script.js one two
[
'/home/jordan/.nvm/versions/node/v16.13.0/bin/node',
'/home/jordan/Programming/Projects/pulp/script.js',
'one',
'two'
]
$ node -e "$(cat script.js)" one two
[ '/home/jordan/.nvm/versions/node/v16.13.0/bin/node', 'one', 'two' ]
$ node -e "$(cat script.js)" -- one two
[ '/home/jordan/.nvm/versions/node/v16.13.0/bin/node', 'one', 'two' ]Since most CLI parsing will call |
a970b1a to
bcbfde1
Compare
- browserify uses CJS, so it will fail - build --optimize/--to uses `purs bundle` - psc-package is not yet supported - `pulp test --runtime` uses `--to` flag - all other tests use `--optimize`/`--to` flag in test
f80c406 to
89b3d64
Compare
"real" versions in that they don't point to github repos' branches
|
CI builds now. I've pushed a few other commits that clean up a few things. I don't think that will cause any issues. |
|
K. This is ready for a review. CI builds with the |
src/Pulp/Run.purs
Outdated
| if (dropPreRelBuildMeta psVer) < psVersions.v0_15_0 then | ||
| { makeEntry: makeCjsEntry, suffix: ".js" } | ||
| else | ||
| { makeEntry: makeEsEntry fullPath, suffix: ".mjs" } |
There was a problem hiding this comment.
Is the .mjs suffix required? What is Spago doing for its script?
There was a problem hiding this comment.
Well, here's our options for this.
- We call
nodeusing the--evalarg and it evaluates the string you give as a script. To make that work on ES modules, we include the--input-type moduleflag, so that the whole code looks like this:node --input-type module --eval "import { main } from 'file:///path/to/rootDir/output/Module.Name/index.js'; main();". Unfortunately, this is problematic as described in an earlier comment - We use
.jsand have to also add apackage.jsonfile that has{"type": "module"}as its contents in the directory containing it, so that Node interprets that JS file as ES modules rather than CommonJS files. AFAIK, this is likely fine sincepurscreates that 'package.json` file - We use
.mjsbecause Node will immediately understand that it's ES modules, regardless of whether thepackage.jsonfile is there.
I went with 3 mainly because it removed an additional problem that could arise. Now that CI builds, I think it's worth it to try the .js version.
There was a problem hiding this comment.
Also, spago stopped using files because I thought using node --eval would work until I came across the CLI args issue. See purescript/spago#865 for more info.
There was a problem hiding this comment.
If we can do just the .js version I'd prefer that.
There was a problem hiding this comment.
Welp. Looks like I might be wrong about 3. https://github.com/purescript-contrib/pulp/runs/5488535135?check_suite_focus=true#step:8:583
There was a problem hiding this comment.
Yup, the package.json file is there, but perhaps node isn't treating it as ES modules because the root dir's package.json file doesn't have type: module? And it can't yet because pulp is being compiled with v0.14.5, not v0.15.0.
I'm going to revert this back to .mjs because that at least builds.
There was a problem hiding this comment.
I took a look at this again. I think the issue is that we're not passing in the --experimental-modules flag to Node when Node is < 13.0.0. See working-group-purescript-es/purescript#11 where this same issue arose.
So, I installed Node 12.6.0 and ran a pulp run test locally. However, I still got an error. So, I tried out a simple example to see if there was something else going on:
$ cat psOutput.js
export const main = function () {
console.log("this works");
}
$ cat entryPoint.js
import { main } from '/home/jordan/Programming/Projects/pulp/psOutput.js';
main();
$ node --version
v12.6.0
$ node --experimental-modules entryPoint.js
(node:28551) ExperimentalWarning: The ESM module loader is experimental.
/home/jordan/Programming/Projects/pulp/entryPoint.js:1
import { main } from 'file:///home/jordan/Programming/Projects/pulp/entryPoint.js';
^
SyntaxError: Unexpected token {
at Module._compile (internal/modules/cjs/loader.js:720:23)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:787:10)
at Module.load (internal/modules/cjs/loader.js:643:32)
at Function.Module._load (internal/modules/cjs/loader.js:556:12)
at internal/modules/esm/translators.js:87:15
at Object.meta.done (internal/modules/esm/create_dynamic_module.js:48:9)
at file:///home/jordan/Programming/Projects/pulp/entryPoint.js:9:13
at ModuleJob.run (internal/modules/esm/module_job.js:111:37)
at processTicksAndRejections (internal/process/task_queues.js:85:5)
at async Loader.import (internal/modules/esm/loader.js:134:24)
I tried import Main ... and import * as Main ..., and still got the same error.
There was a problem hiding this comment.
I shifted the files around a bit
$ ls z/
entryPoint.js package.json psOut.js
$ cat z/package.json
{"type": "module"}
$ node --experimental-modules z/entryPoint.js
(node:29562) ExperimentalWarning: The ESM module loader is experimental.
this worksSo, using .js can work. The issue is the package.json file's location.
There was a problem hiding this comment.
OK! Figured it out. The issue is that the script generated by pulp is stored in the /tmp/ directory. Since that directory doesn't contain a package.json file with {"type": "module"} as its content, Node throws the above error. Once I create a package.json file there, Node runs fine.
| // For tests to pass on the `v0.15.0-alpha-01` purs version, | ||
| // we need to use a custom fork of a non-core repo | ||
| // (i.e. working-group-purescript-es/purescript-prelude#es-modules-libraries). | ||
| // Since those repos' `bower.json` files point to dependencies | ||
| // whose "versions" are branch names as well, the `bower.json` JSON parsing fails. | ||
| // So, we need to remove the `bower_components` folder that gets set up | ||
| // when `pulp init` is called, and re-install the bower deps | ||
| // using the `bower.json` file. | ||
| // | ||
| // Furthermore, we have to install all 3 deps used in a typical `pulp init` | ||
| // setup. Otherwise, `pulp version` fails due to other dependency-related reasons. |
There was a problem hiding this comment.
What should happen once we no longer have #es-modules-libraries branches?
There was a problem hiding this comment.
It's an unfortunate a cyclical dependency. When bower install is run by pulp init, it downloads the repos and stores their bower.json file as .bower.json. At some point, this file is parsed as a bower file. However, that parsing only works if the version for a dependency follows the typical semver variant (e.g. "purescript-prelude": "^7.0.0"), not the repo-branch variant (e.g. "purescript-prelude": "working-group-purescript-es/purescript-prelude#es-modules-libraries").
So, we'd need to make a release for prelude, effect, console, and psci-support where their bower.json file's dependencies versions point to actual versions (e.g. ^7.0.0), update Init.purs to install those versions rather than the es-modules-libraries branch, and then drop this code.
There was a problem hiding this comment.
OK. Then when merging this we should create an issue to come back and fix this, because it'll break later.
thomashoneyman
left a comment
There was a problem hiding this comment.
On the whole this looks good; I have some comments wrt the error messages and how we are going to handle updating Pulp once we have proper library releases that are compatible with 0.15.
|
If we can resolve the |
This reverts commit 1b97367.
Fixes #400.