Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Split export extensions into 2 #706

Closed
4 tasks
hzoo opened this issue Aug 29, 2017 · 19 comments
Closed
4 tasks

Split export extensions into 2 #706

hzoo opened this issue Aug 29, 2017 · 19 comments

Comments

@hzoo
Copy link
Member

hzoo commented Aug 29, 2017

So this is more of a refactor

exportExtensions:
Proposal 1: export v from "mod"
Proposal 2: export * as ns from "mod"

Like in babel/babel#6080 except for the babylon plugin.

exportDefault?
exportNamespace?


  • Comment below you are going to do this (for others to know)
  • Read CONTRIBUTING.md
  • Split the plugins into 2
  • fix all the test fixtures (may need 2 folders, pass the correct option down)
@rajikaimal
Copy link

Would like to work on this !

@swapnilraj
Copy link

Hi I would like to help with this.👍

@zoecarver
Copy link
Contributor

zoecarver commented Sep 19, 2017

I am not sure how far above my level this is, but I would like to give it a shot. @hzoo just so I understand correctly, What this is trying to do is change something like this:

export default () => {
    throw new Error('test');
};

Out:

Error: test
    at exports.default

into something like this:

let baz = () => {
    throw new Error('test');
};

export default baz;

Out:

Error: test
    at baz

@existentialism
Copy link
Member

existentialism commented Sep 19, 2017

@pudility sounds good, but do you mind if we check if @rajikaimal has made any progress first (just want to make sure they didn't make a bunch of effort on it)?

@rajikaimal friendly ping! :)

@zoecarver
Copy link
Contributor

Definitely, how should we check?

@jridgewell
Copy link
Member

Just see if he responds. If not by tomorrow, I'd say go for it.

@zoecarver
Copy link
Contributor

Great! Will do.

@zoecarver
Copy link
Contributor

Before I set up my babylon environment and test it more thoroughly, I would really appreciate it if someone would look over my code just to make sure that I am doing everything correctly. Thank you!

@existentialism
Copy link
Member

existentialism commented Sep 20, 2017

@pudility just to be clear, this task is to split up the exportExtensions plugin inside of Babylon into exportDefault and exportNamespace since they are two separate proposals.

Check out the link to CONTRIBUTING in the original post by @hzoo for info on adding a new plugin. The Babel side of this was completed in babel/babel#6080 (but we'll probably need to follow up with two different syntax-enabling plugins once this work is done).

@zoecarver
Copy link
Contributor

So the thing I made already exists, but what still needs to be made is something for exportNamespaces? For example, something like this:

exports.hello = () => {}

@hzoo
Copy link
Member Author

hzoo commented Sep 20, 2017

@pudility this task is a refactor; there is no behavior to implement - we need to split the logic.

parseExport(node: N.Node): N.Node {
// export * from '...'
if (this.shouldParseExportStar()) {
this.parseExportStar(node, this.hasPlugin("exportExtensions"));
if (node.type === "ExportAllDeclaration") return node;
} else if (
this.hasPlugin("exportExtensions") &&
this.isExportDefaultSpecifier()
) {
so that this.hasPlugin("exportExtensions") is split

@zoecarver
Copy link
Contributor

zoecarver commented Sep 20, 2017

Alright, I think I understand. The only thing I am still confused on is what it needs to be split into. Do you mean that it should be accessible from two different hasPlugins? Like this:
this.hasPlugin("exportExtensions") and this.hasPlugin("exportNamespace") instead of just this.hasPlugin("exportExtensions").

@hzoo
Copy link
Member Author

hzoo commented Sep 20, 2017

exportextensions used to be 1 proposal and now its 2.

export-default-from babel/proposals#17
export-ns-from babel/proposals#16

they are independent and at different stages so we should split them up. you just need to change this.hasPlugin to use 2 new strings, update the readme/docs, and update the folders/tests

@zoecarver
Copy link
Contributor

Ok, thank you so much for explaining, I now get it. I will give it a shot, but I am not quite sure if I will be able to do it fully.

@zoecarver
Copy link
Contributor

@hzoo I think this may be a bit above my current skill level. I am going to keep looking through the code, but I don't think that I will be able to solve this issue. Thanks again for the help.

@jridgewell
Copy link
Member

It should be as simple as using two separate plugin strings. Right now, we have one string ("exportExtensions") that enables two separate features. We want to remove that string, and instead replace it with ("exportDefault" and "exportNamespace").

@zoecarver
Copy link
Contributor

zoecarver commented Sep 20, 2017

Oh, Thats simpler than I thought. I just added that, I will update the docs and then create a PR. Thanks so much @jridgewell !

@zoecarver
Copy link
Contributor

zoecarver commented Sep 21, 2017

So I have modified the code to now look like this:

  parseExport(node: N.Node): N.Node {
    // export * from '...'
    if (this.shouldParseExportStar()) {
      this.parseExportStar(node, (
        this.hasPlugin("exportDefault") ||
        this.hasPlugin("exportNamespace")
      ));
      if (node.type === "ExportAllDeclaration") return node;
    } else if (this.hasPlugin("exportDefault")) {

and I replace export-expressions with export-default and export-namespace. I also changed the options.json file in both of them to use the plug in "exportDefault" and "exportNamespace" respectively. I think that I must have done something wrong here (probably when modifying statement.js) because when I run yarn run test-only I get three errors:

  4223 passed
  3 failed
  112 skipped

  indexexperimental/export default/default

  /Users/zoe/Developer/babylon/split-export/babylon/test/helpers/runFixtureTests.js:21

   20:               name + "/" + task.actual.filename + ": " + err.message;
   21:             t.fail(message);
   22:           }

  experimental/export-default/default/actual.js: Unexpected token, expected { (1:7)

plus two almost identical errors:
2. experimental/export-namespace/ns/actual.js: Unexpected token (1:9)
3. experimental/export-default/default-type-without-flow/actual.js: Unexpected token, expected { (1:7)


Thank you all so much for helping me create my first real contribution to Babylon, I really like getting to learn the process for all of this stuff.

@babel-bot
Copy link

This issue has been moved to babel/babel#6678.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants