Skip to content

Conversation

@JordanMartinez
Copy link
Contributor

@JordanMartinez JordanMartinez commented Mar 4, 2022

Fixes #400.

@JordanMartinez
Copy link
Contributor Author

JordanMartinez commented Mar 4, 2022

Let's say script.js is the following JavaScript code:

const p = require("process");
console.log(p.argv);

Using node --eval doesn't provide the same argv as using node script.js:

$ 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 Array.drop 2 <$> Process.argv, using node -e will actually break PureScript projects that are CLI programs because the first argument is missing.

@JordanMartinez JordanMartinez force-pushed the support-es-modules branch 2 times, most recently from a970b1a to bcbfde1 Compare March 7, 2022 17:55
@JordanMartinez
Copy link
Contributor Author

JordanMartinez commented Mar 8, 2022

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.

@JordanMartinez
Copy link
Contributor Author

K. This is ready for a review. CI builds with the v0.15.0-alpha-01 release now.

if (dropPreRelBuildMeta psVer) < psVersions.v0_15_0 then
{ makeEntry: makeCjsEntry, suffix: ".js" }
else
{ makeEntry: makeEsEntry fullPath, suffix: ".mjs" }
Copy link
Contributor

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?

Copy link
Contributor Author

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.

  1. We call node using the --eval arg and it evaluates the string you give as a script. To make that work on ES modules, we include the --input-type module flag, 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
  2. We use .js and have to also add a package.json file 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 since purs creates that 'package.json` file
  3. We use .mjs because Node will immediately understand that it's ES modules, regardless of whether the package.json file 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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@JordanMartinez JordanMartinez Mar 9, 2022

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 works

So, using .js can work. The issue is the package.json file's location.

Copy link
Contributor Author

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.

Comment on lines +668 to +678
// 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.
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #402

Copy link
Contributor

@thomashoneyman thomashoneyman left a 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.

@thomashoneyman
Copy link
Contributor

If we can resolve the .js extension and create an issue with regards to updating the bower.json dependencies once libraries have their 0.15 releases then I'm good to go with this!

Co-authored-by: Thomas Honeyman <hello@thomashoneyman.com>
@JordanMartinez JordanMartinez merged commit ac0bd65 into master Mar 11, 2022
@JordanMartinez JordanMartinez deleted the support-es-modules branch March 11, 2022 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Add support for upcoming 0.15.0 PureScript release

3 participants