This repository has been archived by the owner. It is now read-only.

Disallow duplicate named exports #107

Merged
merged 1 commit into from Sep 22, 2016

Conversation

Projects
None yet
5 participants
@kaicataldo
Member

kaicataldo commented Sep 5, 2016

I think I got all the cases for named exports - let me know if I missed one! Also wasn't sure about the error message wording, but was hoping that by including the name of the offending duplicate export that debugging could be made easier for the end user. Finally, wasn't sure where to put the tests - happy to move them!

@@ -1,2 +1,2 @@
export interface foo { p: number };
export interface foo<T> { p: T };
export interface bar<T> { p: T };

This comment has been minimized.

@kaicataldo

kaicataldo Sep 5, 2016

Member

I'm pretty new to Flow - let me know if this isn't the correct fix for this test

@kaicataldo

kaicataldo Sep 5, 2016

Member

I'm pretty new to Flow - let me know if this isn't the correct fix for this test

This comment has been minimized.

@danez

danez Sep 21, 2016

Member

This is correct

@danez

danez Sep 21, 2016

Member

This is correct

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Sep 5, 2016

Member

Looks like the CI tests are failing because of test files in Babel plugins that contain duplicates exports in the same file. I think it'll require similar changes to this PR: babel/babel#3518.

How are cross-repo changes like this usually handled? I'm happy to make the necessary changes in the Babel repo, including reverting the band-aid fix for duplicate default exports if and when this PR lands.

Member

kaicataldo commented Sep 5, 2016

Looks like the CI tests are failing because of test files in Babel plugins that contain duplicates exports in the same file. I think it'll require similar changes to this PR: babel/babel#3518.

How are cross-repo changes like this usually handled? I'm happy to make the necessary changes in the Babel repo, including reverting the band-aid fix for duplicate default exports if and when this PR lands.

@danez

This comment has been minimized.

Show comment
Hide comment
@danez

danez Sep 5, 2016

Member

Good job. That looks really good. The flow test looks good. 👍

The tests of babel now fail, but this is okay for now, but we would need to correct them at some point, before we release this.

Member

danez commented Sep 5, 2016

Good job. That looks really good. The flow test looks good. 👍

The tests of babel now fail, but this is okay for now, but we would need to correct them at some point, before we release this.

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Sep 5, 2016

Member

Cool! Should I go ahead and make a PR to the main babel repo?

Member

kaicataldo commented Sep 5, 2016

Cool! Should I go ahead and make a PR to the main babel repo?

@danez

This comment has been minimized.

Show comment
Hide comment
@danez

danez Sep 5, 2016

Member

Yes that would be nice. Thanks.

Oh for some reason I did not see the other comment of you. So there is no real workflow yet on how to do the cross-repo stuff. I described in this PR how to link babylon in babel. I should really but that in the Contributing file.

babel/babel#3700 (comment)

Member

danez commented Sep 5, 2016

Yes that would be nice. Thanks.

Oh for some reason I did not see the other comment of you. So there is no real workflow yet on how to do the cross-repo stuff. I described in this PR how to link babylon in babel. I should really but that in the Contributing file.

babel/babel#3700 (comment)

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Sep 5, 2016

Member

Thanks for all the info on this! One more question - do you think I should split the changes into batches? Splitting each one of those files into multiple files is going to be a pretty massive diff.

Member

kaicataldo commented Sep 5, 2016

Thanks for all the info on this! One more question - do you think I should split the changes into batches? Splitting each one of those files into multiple files is going to be a pretty massive diff.

@danez

This comment has been minimized.

Show comment
Hide comment
@danez

danez Sep 13, 2016

Member

Ah sorry, for not answering. As long as the changes belong together I think they should go into the same PR imho. And I think this would account for this case.

Member

danez commented Sep 13, 2016

Ah sorry, for not answering. As long as the changes belong together I think they should go into the same PR imho. And I think this would account for this case.

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Sep 19, 2016

Member

Finally started in on this, though might take me a few days. Doing it in chunks because there are a lot of changes.

Member

kaicataldo commented Sep 19, 2016

Finally started in on this, though might take me a few days. Doing it in chunks because there are a lot of changes.

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Sep 21, 2016

Member

Finally got around to fixing those tests! PR open here: babel/babel#4538

I mention it in the issue as well, but will also make a 2nd PR that removes the initial band-aid fix that was merged into Babel itself and to rename the test files so they follow the convention used in the currently open PR. I plan on getting to this tomorrow!

Member

kaicataldo commented Sep 21, 2016

Finally got around to fixing those tests! PR open here: babel/babel#4538

I mention it in the issue as well, but will also make a 2nd PR that removes the initial band-aid fix that was merged into Babel itself and to rename the test files so they follow the convention used in the currently open PR. I plan on getting to this tomorrow!

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 21, 2016

Current coverage is 97.04% (diff: 100%)

Merging #107 into master will increase coverage by 0.27%

@@             master       #107   diff @@
==========================================
  Files            19         19          
  Lines          3130       3185    +55   
  Methods         320        327     +7   
  Messages          0          0          
  Branches        800        819    +19   
==========================================
+ Hits           3029       3091    +62   
+ Misses          101         94     -7   
  Partials          0          0          

Powered by Codecov. Last update dc56c0b...030dc4c

codecov-io commented Sep 21, 2016

Current coverage is 97.04% (diff: 100%)

Merging #107 into master will increase coverage by 0.27%

@@             master       #107   diff @@
==========================================
  Files            19         19          
  Lines          3130       3185    +55   
  Methods         320        327     +7   
  Messages          0          0          
  Branches        800        819    +19   
==========================================
+ Hits           3029       3091    +62   
+ Misses          101         94     -7   
  Partials          0          0          

Powered by Codecov. Last update dc56c0b...030dc4c

@@ -844,7 +844,7 @@ pp.parseExport = function (node) {
}
node.declaration = expr;
if (needsSemi) this.semicolon();
this.checkExport(node);
this.checkExport(node, true, true);

This comment has been minimized.

@hzoo

hzoo Sep 21, 2016

Member

just a thought: might be nice if this method wasn't taking in true, true (an object, named, etc) but I know we do that everywhere else ^ this.parseFunction(expr, true, false, false, true); 😄

@hzoo

hzoo Sep 21, 2016

Member

just a thought: might be nice if this method wasn't taking in true, true (an object, named, etc) but I know we do that everywhere else ^ this.parseFunction(expr, true, false, false, true); 😄

This comment has been minimized.

@kaicataldo

kaicataldo Sep 21, 2016

Member

Yeah, was trying to follow the style I saw elsewhere, but agree. Would you like me to change that to an object?

@kaicataldo

kaicataldo Sep 21, 2016

Member

Yeah, was trying to follow the style I saw elsewhere, but agree. Would you like me to change that to an object?

This comment has been minimized.

@danez

danez Sep 21, 2016

Member

@hzoo: Do you mean checkExport(node: Node, options: { named: boolean, default: boolean }) or what do you mean with object? Maybe also checkExport(node: Node, type: "named" | "default" | null)?

@danez

danez Sep 21, 2016

Member

@hzoo: Do you mean checkExport(node: Node, options: { named: boolean, default: boolean }) or what do you mean with object? Maybe also checkExport(node: Node, type: "named" | "default" | null)?

This comment has been minimized.

@hzoo

hzoo Sep 21, 2016

Member

yeah to make it more readable - dono if it was done before for perf?

@hzoo

hzoo Sep 21, 2016

Member

yeah to make it more readable - dono if it was done before for perf?

This comment has been minimized.

@kaicataldo

kaicataldo Sep 21, 2016

Member

I assumed it was done this way for performance reasons

@kaicataldo

kaicataldo Sep 21, 2016

Member

I assumed it was done this way for performance reasons

@danez

danez approved these changes Sep 21, 2016

@@ -844,7 +844,7 @@ pp.parseExport = function (node) {
}
node.declaration = expr;
if (needsSemi) this.semicolon();
this.checkExport(node);
this.checkExport(node, true, true);

This comment has been minimized.

@danez

danez Sep 21, 2016

Member

@hzoo: Do you mean checkExport(node: Node, options: { named: boolean, default: boolean }) or what do you mean with object? Maybe also checkExport(node: Node, type: "named" | "default" | null)?

@danez

danez Sep 21, 2016

Member

@hzoo: Do you mean checkExport(node: Node, options: { named: boolean, default: boolean }) or what do you mean with object? Maybe also checkExport(node: Node, type: "named" | "default" | null)?

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Sep 21, 2016

Member

Opened one more PR that reverts the commit I made that throws a warning in babel itself since we have moved that into babylon. I also updated the test names so that they match the convention I used in my more recent PR that was merged today. Let me know if you need anything else!

Member

kaicataldo commented Sep 21, 2016

Opened one more PR that reverts the commit I made that throws a warning in babel itself since we have moved that into babylon. I also updated the test names so that they match the convention I used in my more recent PR that was merged today. Let me know if you need anything else!

@hzoo hzoo merged commit 650e333 into babel:master Sep 22, 2016

3 checks passed

codecov/patch 100% of diff hit (target 96.77%)
Details
codecov/project 97.04% (+0.27%) compared to dc56c0b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kaicataldo kaicataldo deleted the kaicataldo:disallow-duplicate-exports branch Sep 22, 2016

@dlmr

This comment has been minimized.

Show comment
Hide comment
@dlmr

dlmr Sep 22, 2016

Seems like this change causes Babylon to throw for one of my files that worked fine before.

The file in questions looks like this:

export toString from './toString';
`toString` has already been exported. Exported identifiers must be unique. (1:7)
> 1 | export toString from './toString';
    |        ^
  2 |

Is this an intended change, is not toString allowed any more?

Seems like all keys on Object.prototype causes this error.

dlmr commented Sep 22, 2016

Seems like this change causes Babylon to throw for one of my files that worked fine before.

The file in questions looks like this:

export toString from './toString';
`toString` has already been exported. Exported identifiers must be unique. (1:7)
> 1 | export toString from './toString';
    |        ^
  2 |

Is this an intended change, is not toString allowed any more?

Seems like all keys on Object.prototype causes this error.

@@ -913,6 +937,20 @@ pp.checkExport = function (node) {
}
};
pp.checkDuplicateExports = function(node, name, isDefault) {
if (this.state.exportedIdentifiers[name]) {

This comment has been minimized.

@dlmr

dlmr Sep 22, 2016

I think we need to manage Object.prototype here.

Something along the lines of this maybe?

if (Object.getOwnPropertyNames(this.state.exportedIdentifiers).indexOf(name) !== -1) {
@dlmr

dlmr Sep 22, 2016

I think we need to manage Object.prototype here.

Something along the lines of this maybe?

if (Object.getOwnPropertyNames(this.state.exportedIdentifiers).indexOf(name) !== -1) {

This comment has been minimized.

@danez

danez Sep 22, 2016

Member

You are right, I wrap up a fix

@danez

danez Sep 22, 2016

Member

You are right, I wrap up a fix

This comment has been minimized.

@hzoo

hzoo Sep 22, 2016

Member

Ah interesting, thanks for the report

cc @kaicataldo

@hzoo

hzoo Sep 22, 2016

Member

Ah interesting, thanks for the report

cc @kaicataldo

This comment has been minimized.

@kaicataldo

kaicataldo Sep 22, 2016

Member

Oh wow, that makes sense. @danez Let me know if you need me work on a fix - happy to do it.

@kaicataldo

kaicataldo Sep 22, 2016

Member

Oh wow, that makes sense. @danez Let me know if you need me work on a fix - happy to do it.

This comment has been minimized.

@hzoo

hzoo Sep 22, 2016

Member

Same issue in jscs-dev/node-jscs#1204 😄

@hzoo

hzoo Sep 22, 2016

Member

Same issue in jscs-dev/node-jscs#1204 😄

danez added a commit to danez/babylon that referenced this pull request Sep 22, 2016

@danez danez referenced this pull request Sep 22, 2016

Merged

Fix duplicate check #137

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