Have module re-exports appear in generated code #3883

citizengabe · 2020-05-19T14:31:37Z

Resolves #1888. See that issue and this discussion on Discourse for background.

The changes in this PR are two-fold:

Add a new field on Language.PureScript.CoreFn.Module to hold re-export data so it can be used during code generation
Use that data to import the necessary modules and export the relevant members in the generated code.

I added a test for parsing the re-exports, but no others. Is there somewhere we test the output of Language.PureScript.CodeGen.JS.moduleToJs?

I'd also like to add this GitHub account to CONTRIBUTORS.md, but am waiting on guidance from my company as to how to reference it as this work was done on company time.

We need to make the `ExportSource` from `Language.PureScript.AST.Declarations.ReExportRef` available during code-gen. This requires adding it to `Module`. We don't actually do the work of adding the data in this commit, we merely add the field, update the JSON codecs, appease the compiler, and add a test for parsing the new field.

citizengabe · 2020-05-19T15:34:11Z

I have some tests that are failing in CI that weren't failing locally. I cleared .psci_modules, .test_modules, and output and ran again locally and got similar failures. I'll investigate and push up a fix.

citizengabe · 2020-05-19T16:05:45Z

It was an issue with bundling. I was handling regular dot notation for exported member access, but not bracket notation.

citizengabe · 2020-05-21T16:29:48Z

I'd also like to add this GitHub account to CONTRIBUTORS.md, but am waiting on guidance from my company as to how to reference it as this work was done on company time.

This is done, so the PR is ready to merge pending review.

joneshf-cn

This seems to do what it says it does, but I don't know enough about module re-exports to approve or reject. @hdgarrood, can you take a peek when you get a second?

kritzcreek · 2020-05-22T11:36:39Z

I'd also like to check that all the re-exporting logic in the IDE still works with this change. Please don't merge before I've approved. I'll check this weekend!

citizengabe · 2020-05-22T14:28:39Z

I'd also like to check that all the re-exporting logic in the IDE still works with this change. Please don't merge before I've approved. I'll check this weekend!

That sounds prudent, but know that the changes are additive only. Nothing was changed WRT externs, and moduleReExports is an additional field. The rest of Language.PureScript.CoreFn.Module.Module and its contents remain unchanged.

kritzcreek · 2020-05-23T06:42:36Z

Yeah, after checking none of these changes affect the IDE. All good from my side.

hdgarrood

Is there somewhere we test the output of Language.PureScript.CodeGen.JS.moduleToJs?

We mostly don't do unit testing, testing is generally done end-to-end via tests/purs/{passing,failing,warning}. Perhaps we could test this by having a module re-export something, having a separate module import that module via the FFI, and test that the re-exports are present in the generated JS that way?

CONTRIBUTORS.md

src/Language/PureScript/CoreFn/Desugar.hs

Now that we have a field for them, we add the re-exports to the module so they show up as both imported modules and exported members during code-gen.

Now that we're threading the necessary information down to `moduleToJs`, we can use it to generate the imports we need to later re-export.

We build up the key/value pairs for the re-exports and append them to the rest of the exports. This broke bundling and required allowing `<module name>.<member name>` in addition to `$foreign.<member name>`. This is the only place where I'm unsure of my changes.

In the last commit, we handled the case of member access using dot notation, but the passing tests were a fluke of caching. Now both cases are handled and tests are passing.

I have a separate account that I use for work and prefer to include this in addition to my personal acccount to differentiate who owns the copyright for what work. I also add my employer to the list of companies holding copyright.

Previously, it was possible to forget to handle new constructors added to `DeclarationRef`. Now we handle them explicitly thus eliminating this potential source of bugs.

hdgarrood · 2020-05-26T18:29:02Z

Don't worry about it now, but for future reference could you please just push new commits to the branch after review? I'd like to be able to review what's changed since I last looked, and we will tidy up the history when merging.

citizengabe · 2020-05-26T18:30:56Z

Don't worry about it now, but for future reference could you please just push new commits to the branch after review?

Sure thing!

hdgarrood · 2020-05-26T18:35:29Z

Looks great! If you could write a test along the lines of what I wrote here: #3883 (review) that would be ideal, then I'm happy to approve.

@hdgarrood

As suggested by @hdgarrood in purescript#3883 (review), we use the FFI to import a re-exported value. Module `A` defines a value `a`, module `B` re-exports `A`, and module `Main` does a foreign import of `a` via `B`. Logging the imported value proves that the re-export chain was successful.

hdgarrood · 2020-05-26T20:43:15Z

Oh wait, sorry, just a couple more cases that I've just remembered about that I think would be worth testing:

re-exporting a re-export
re-exporting something from a module if we have a data constructor with the same name in scope (to make sure this continues to work well together with the module name renaming mnLookup thing, in CodeGen/JS.hs), eg

module B (module A, A(..)) where

import A
data A = A

to make sure the exports list refers to A_1, which is what the module A should be imported as in the JS, rather than just A.

citizengabe · 2020-05-26T20:52:51Z

Would you prefer separate test suites for the different cases or is a single suite sufficient?

hdgarrood · 2020-05-26T20:55:43Z

Whatever's easiest - I think putting them all under tests/purs/passing/ReExportsExported is fine.

We tested that the value defined in one module could be re-exported and then imported via FFI. Now, we test that: 1. a re-exported module doesn't cause a name clash with any data constructors defined in the re-exporting module 2. a value, `a`, defined in a module, `A`, may be re-exported from another module, `B`, and again re-exported from a third module, `C`, which imported `a` from `B`.

hdgarrood

Thanks!

I forgot to export the data constructor `A`. I had visually inspected the generated code and saw that the names didn't conflict. However, it's better to have the test make this assurance.

citizengabe · 2020-06-08T16:10:24Z

@hdgarrood is there anything preventing this PR from being merged?

hdgarrood · 2020-06-08T16:15:33Z

Our policy is that PRs require at least two core team approvals (if the person who submits the PR is already a core team member, that counts).

citizengabe · 2020-06-09T17:50:05Z

@kritzcreek You took a look a couple of weeks ago. Does it still look good?

kritzcreek · 2020-06-13T22:00:55Z

You took a look a couple of weeks ago. Does it still look good?

Sorry, I was on vacation last week. Change still looks good to me 👍

citizengabe · 2020-06-14T02:07:39Z

Thank you both for the reviews!

Sorry, I was on vacation last week.

Hope it was a good one 😄

hedefalk · 2020-06-17T13:31:33Z

Oooh, think I struck this and asked in slack https://functionalprogramming.slack.com/archives/C717K38CE/p1592399197296900

Does this solve the issue I'm having right now when trying to re-export for commonjs:

Foo.purs:

module Foo ( module Foo ) where
import Bar (bar) as Foo

Bar.purs:

module Bar where
bar :: String
bar = "bar"

npx spago bundle-module --main Foo gives me:

// Generated by purs bundle 0.13.8
var PS = {};
module.exports = PS["Foo"];

joneshf-cn reviewed May 22, 2020

View reviewed changes

hdgarrood reviewed May 25, 2020

View reviewed changes

CONTRIBUTORS.md Outdated Show resolved Hide resolved

hdgarrood reviewed May 25, 2020

View reviewed changes

src/Language/PureScript/CoreFn/Desugar.hs Outdated Show resolved Hide resolved

src/Language/PureScript/CoreFn/Desugar.hs Outdated Show resolved Hide resolved

citizengabe force-pushed the citizengabe/FBCM-3329/have-module-reexports-appear-in-generated-code branch from 6fd4be2 to 653d783 Compare May 26, 2020 16:11

citizengabe added 6 commits May 26, 2020 13:21

Add source of re-export to the module

e35fe5b

Now that we have a field for them, we add the re-exports to the module so they show up as both imported modules and exported members during code-gen.

Generate imports for re-exports

7ae25b6

Now that we're threading the necessary information down to `moduleToJs`, we can use it to generate the imports we need to later re-export.

Handle bracket notation for re-exports while bundling

abac33d

In the last commit, we handled the case of member access using dot notation, but the passing tests were a fluke of caching. Now both cases are handled and tests are passing.

Handle Language.PureScript.CoreFn.exportToCoreFn cases explicitly

24df28d

Previously, it was possible to forget to handle new constructors added to `DeclarationRef`. Now we handle them explicitly thus eliminating this potential source of bugs.

citizengabe force-pushed the citizengabe/FBCM-3329/have-module-reexports-appear-in-generated-code branch from 653d783 to 24df28d Compare May 26, 2020 18:26

hdgarrood approved these changes May 27, 2020

View reviewed changes

Have test module export data constructor in addition to re-export

76a52bf

I forgot to export the data constructor `A`. I had visually inspected the generated code and saw that the names didn't conflict. However, it's better to have the test make this assurance.

kritzcreek approved these changes Jun 13, 2020

View reviewed changes

hdgarrood merged commit c044d69 into purescript:master Jun 13, 2020

kl0tl mentioned this pull request Mar 21, 2021

ES modules #3791

Closed

JordanMartinez mentioned this pull request Jan 18, 2022

Update version to v0.15.0 working-group-purescript-es/purescript#4

Closed

Have module re-exports appear in generated code #3883

Have module re-exports appear in generated code #3883

Uh oh!

Conversation

citizengabe commented May 19, 2020

Uh oh!

citizengabe commented May 19, 2020

Uh oh!

citizengabe commented May 19, 2020

Uh oh!

citizengabe commented May 21, 2020

Uh oh!

joneshf-cn left a comment

Choose a reason for hiding this comment

Uh oh!

kritzcreek commented May 22, 2020

Uh oh!

citizengabe commented May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kritzcreek commented May 23, 2020

Uh oh!

hdgarrood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hdgarrood commented May 26, 2020

Uh oh!

citizengabe commented May 26, 2020

Uh oh!

hdgarrood commented May 26, 2020

Uh oh!

hdgarrood commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

citizengabe commented May 26, 2020

Uh oh!

hdgarrood commented May 26, 2020

Uh oh!

hdgarrood left a comment

Choose a reason for hiding this comment

Uh oh!

citizengabe commented Jun 8, 2020

Uh oh!

hdgarrood commented Jun 8, 2020

Uh oh!

citizengabe commented Jun 9, 2020

Uh oh!

kritzcreek commented Jun 13, 2020

Uh oh!

citizengabe commented Jun 14, 2020

Uh oh!

hedefalk commented Jun 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

citizengabe commented May 22, 2020 •

edited

Loading

hdgarrood commented May 26, 2020 •

edited

Loading