Skip to content
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

Migrate FFI to ES modules #287

Conversation

JordanMartinez
Copy link
Contributor

@JordanMartinez JordanMartinez commented Mar 4, 2022

Description of the change

Backlinking to purescript/purescript#4244

Migrates FFI to ES modules. This continues what was started in #284 but using a different branch named es-modules-libraries to ensure further development on the compiler isn't affected by commits pushed to this PR.

This PR also succeeds #264 since that PR was not opened by the 'working-group-purescript-es' org, which allows multiple people to push to the PR if the original submitter is gone for while.

This PR will fail to build until pulp has been updated to support ES modules when one calls pulp run and pulp test. See purescript-contrib/pulp#400 for more info.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@JordanMartinez JordanMartinez added type: breaking change A change that requires a major version bump. purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 labels Mar 4, 2022
@JordanMartinez
Copy link
Contributor Author

PRs to pulp to support ES modules are ready for review:

@JordanMartinez
Copy link
Contributor Author

CI builds now 🎉

@JordanMartinez
Copy link
Contributor Author

The alpha-02 release is now on GH. Restarting CI to verify this PR still builds.

@JordanMartinez
Copy link
Contributor Author

CI builds on the alpha-02 release.

@thomashoneyman
Copy link
Member

thomashoneyman commented Mar 11, 2022

We need to be careful that setup-purescript has auto-updated to the latest unstable release before re-running CI. For example, it's still on alpha-01:

https://github.com/purescript-contrib/setup-purescript/blob/81d7c48c3f39de23dcd3626ab216699010200db2/dist/versions-v2.json#L2-L4

and so that re-run of CI pulled in the old purs:

Cached path /opt/hostedtoolcache/purs/0.15.0-alpha-01/x64, adding to PATH

I've restarted the sync versions job there so that it updates the file and then we can re-run CI here.

@JordanMartinez
Copy link
Contributor Author

Ah, good catch.

.eslintrc.json Outdated
},
"extends": "eslint:recommended",
"env": {
"commonjs": true
"es6": true
Copy link
Member

Choose a reason for hiding this comment

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

I don't recall if we ended up agreeing on ES6 in the eslint configurations vs. just updating the parserOptions.sourceType field. Do you happen to have a link to that discussion?

If we are going forward with ES6, then I'm curious if we need to set both env: es6 and parserOptions.ecmaVersion: 6. What happens if we just remove env but still set the parser option to ecmaVersion 6?

.eslintrc.json Outdated
},
"extends": "eslint:recommended",
"env": {
"commonjs": true
"es6": true
Copy link
Member

Choose a reason for hiding this comment

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

I don't recall if we ended up agreeing on ES6 in the eslint configurations vs. just updating the parserOptions.sourceType field. Do you happen to have a link to that discussion?

If we are going forward with ES6, then I'm curious if we need to set both env: es6 and parserOptions.ecmaVersion: 6. What happens if we just remove env but still set the parser option to ecmaVersion 6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh... I remember this being important, but I can't recall why. However, running eslint src test doesn't report anything with es6: true removed.

@thomashoneyman
Copy link
Member

This looks good to me other than the discussion around the eslint configuration.

"purescript-psa": "^0.8.0",
"pulp": "^15.0.0",
"purescript-psa": "^0.8.2",
"pulp": "16.0.0-0",
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for the prerelease version of Pulp? Should this be 16.0.0?

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 made a pre-release of 16.0.0 in case we wanted to add support for esbuild later or come across other bugs.

@JordanMartinez JordanMartinez merged commit 80b91b4 into purescript:master Mar 11, 2022
@thomashoneyman
Copy link
Member

@JordanMartinez Is it acceptable to delete the es-modules-libraries branch, or does it need to be kept around for the sake of upstream packages?

@JordanMartinez JordanMartinez mentioned this pull request Mar 11, 2022
4 tasks
@JordanMartinez
Copy link
Contributor Author

The working-group-purescript-es/package-set#main should be using the es-modules branch still, but [email protected] would still try to install the branch via pulp ini. So, perhaps we should hold off until a [email protected] can be released that just installs master from the core repo, not the es-modules-libraries branch from the working group repo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 type: breaking change A change that requires a major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants