-
Notifications
You must be signed in to change notification settings - Fork 82
Support upcoming v0.15.0 compiler #401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the .mjs suffix required? What is Spago doing for its script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can do just the .js version I'd prefer that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should happen once we no longer have #es-modules-libraries branches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Then when merging this we should create an issue to come back and fix this, because it'll break later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #402
thomashoneyman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.