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

fixes #35: adds module import and export grammar #43

Merged
merged 2 commits into from Mar 5, 2015
Merged

Conversation

@caridy
Copy link
Contributor

@caridy caridy commented Jan 27, 2015

This is the bulk of the module grammar from TC39, the exact same supported by esprima today in the harmony branch. This PR includes:

  • import grammar
  • export grammar
  • option sourceType=module to enable strict mode, import, export and any other module's feature.

Export Syntax

// default exports
export default 42;
export default {};
export default [];
export default (1 + 2);
export default foo;
export default function () {}
export default class {}
export default function foo () {}
export default class foo {}

// variables exports
export var foo = 1;
export var foo = function () {};
export var bar;
export let foo = 2;
export let bar;
export const foo = 3;
export function foo () {}
export class foo {}

// named exports
export {};
export {foo};
export {foo, bar};
export {foo as bar};
export {foo as default};
export {foo as default, bar};

// exports from
export * from "foo";
export {} from "foo";
export {foo} from "foo";
export {foo, bar} from "foo";
export {foo as bar} from "foo";
export {foo as default} from "foo";
export {foo as default, bar} from "foo";
export {default} from "foo";
export {default as foo} from "foo";

Import Syntax

// default imports
import foo from "foo";
import {default as foo} from "foo";

// named imports
import {} from "foo";
import {bar} from "foo";
import {bar, baz} from "foo";
import {bar as baz} from "foo";
import {bar as baz, xyz} from "foo";

// glob imports
import * as foo from "foo";

// mixing imports
import foo, {baz as xyz} from "foo";
import foo, * as bar from "foo";

// just import
import "foo";

Other notes

  • More tests, e.g.: import, export declarations are required to be at the top level, etc.
  • Since espree does not support classes as today, we need to revisit export once that lands (I added few TODO lines fro that)
jsx: false,

// enable parsing of import/export declarations
module: false

This comment has been minimized.

@nzakas

nzakas Jan 27, 2015
Member

Do we still need this? I was thinking just scriptType would be enough.

This comment has been minimized.

@caridy

caridy Jan 27, 2015
Author Contributor

True, we could use extra.scriptType==="module" everywhere. The way I'm thinking here is to have the scriptType to alter some of the feature flags based on its value, but keeping the code clean by verifying the actual feature flags. Although, I will be ok with using scriptType directly for now.

This comment has been minimized.

@nzakas

nzakas Jan 28, 2015
Member

I think it's fine to add an isModule flag so you don't need to keep comparing scriptType. It's just having an ecmaFeature of module doesn't make much sense since it can't be independently controlled outside of scriptType'.

@@ -0,0 +1,6 @@
module.exports = {

This comment has been minimized.

@nzakas

nzakas Jan 27, 2015
Member

This test is failing in Travis because it expects to throw an error. Probably need to move this out into ecma-features-mix to avoid that.

This comment has been minimized.

@caridy

caridy Jan 27, 2015
Author Contributor

well, really what this is testing is that scriptType=module should imply script mode where with declaration is forbidden.

This comment has been minimized.

@nzakas

nzakas Jan 28, 2015
Member

Yup, I'm aware. The problem is that every ecmaFeatures test is supposed to throw when not enabled. Since with is allowed even when not using a module, it doesn't throw, so the test "fails". So the only real solution is to move the test outside of this directory so it's not bound by this rule.

@nzakas
Copy link
Member

@nzakas nzakas commented Jan 27, 2015

Overall looks great! Just a couple of notes.

And some bookkeeping stuff:

  1. Can you reformat the commit message to be "New: Add ES6 module support (fixes #35)". It's just the summary convention we have.
  2. Can you sign our CLA? http://eslint.org/cla

Thanks!

@nzakas
Copy link
Member

@nzakas nzakas commented Jan 27, 2015

Forgot to mention, you're right about the tests. I was trying first to get away from having giant files and planned on going back to clean up testing when I got some time. Subdirectories sounds like a great idea.

espree.js Outdated
throwError({}, Messages.IllegalImportDeclaration, "ILLEGAL");
}
return parseImportDeclaration();
case "import":

This comment has been minimized.

@cvrebert

cvrebert Jan 28, 2015

Two case "import":s? That seems wrong.

@xjamundx
Copy link
Contributor

@xjamundx xjamundx commented Jan 28, 2015

doing cartwheels about this 🎠

espree.js Outdated
@@ -4467,6 +4733,11 @@ function parse(code, options) {
extra.trailingComments = [];
extra.leadingComments = [];
}
if (options.sourceType === "module") {
extra.ecmaFeatures.blockBindings = true;

This comment has been minimized.

@nzakas

nzakas Jan 28, 2015
Member

Maybe this should enable all ES6 features, not just block bindings?

This comment has been minimized.

@caridy

caridy Jan 29, 2015
Author Contributor

ok, that makes more sense!

@caridy
Copy link
Contributor Author

@caridy caridy commented Jan 29, 2015

Guys, I will probably get another chance to work on this on friday when flying back to miami after reactconf, be patient.

@nzakas
Copy link
Member

@nzakas nzakas commented Jan 29, 2015

Sure thing, thanks for the update.

@vslinko
Copy link

@vslinko vslinko commented Jan 31, 2015

👍

@caridy
Copy link
Contributor Author

@caridy caridy commented Feb 6, 2015

After a discussion with @jeffmo at TC39 meeting last week, we decided to make one small adjustment (a non-backward compatible change) in esprima to align better with the module grammar, more details here:

jquery/esprima#311

I will incorporate that requirement into this PR.

@mikesherov
Copy link

@mikesherov mikesherov commented Feb 6, 2015

@caridy also note the fix we made in esprima to ensure the correct token lists: jquery/esprima#310

@mikesherov
Copy link

@mikesherov mikesherov commented Feb 6, 2015

@caridy, would you mind submitting the same patch upstream to the harmony branch?

@mikesherov mikesherov mentioned this pull request Feb 6, 2015
0 of 4 tasks complete
espree.js Outdated
// covers:
// export default ...
lex();
if (matchKeyword("function") || matchKeyword("class")) {

This comment has been minimized.

@mikesherov

mikesherov Feb 6, 2015

need to comment out the class match here as well, I suppose?

@caridy
Copy link
Contributor Author

@caridy caridy commented Feb 6, 2015

@caridy also note the fix we made in esprima to ensure the correct token lists: jquery/esprima#310

thanks @mikesherov, I wasn't proud of the rewind() hack, jejejej! I'm glad you fix it, I will incorporate that in this branch.

@sebmck
Copy link

@sebmck sebmck commented Feb 13, 2015

For what it's worth, the following should be illegal

{
  import "import/export outside of the top level";
}

Not sure if this was intentional or not given that I'm not sure if espree is supposed to be "loose" or not given that it's for a linter.

@caridy
Copy link
Contributor Author

@caridy caridy commented Feb 13, 2015

@sebmck yeah, that will be covered, as for the loose, IMO it should always be loose, but that's a good question for @nzakas. If that's the case, I can use throwErrorTolerant instead of throwError.

@nzakas
Copy link
Member

@nzakas nzakas commented Feb 13, 2015

There aren't many tolerant errors, so I'm pretty agnostic as to the severity.

@cvrebert
Copy link

@cvrebert cvrebert commented Feb 13, 2015

For the sake of compatibility with Esprima (see jquery/esprima@b1dd29e), I would request the use of throwErrorTolerant in such cases.

@nzakas
Copy link
Member

@nzakas nzakas commented Feb 14, 2015

Fine by me.

@jonathanKingston
Copy link

@jonathanKingston jonathanKingston commented Feb 14, 2015

@caridy do you have a corresponding commit to pull through sourceType into eslint or some example code?

So far had no other troubles with using this code 👍

@nzakas
Copy link
Member

@nzakas nzakas commented Feb 14, 2015

@jonathanKingston we haven't yet decided how this would be exposed in ESLint. One step at a time. :)

@caridy any ETA for finishing this up?

@mikesherov
Copy link

@mikesherov mikesherov commented Feb 14, 2015

@nzakas, there's still an ongoing discussion to resolve interop between Esprima and acorn and SpiderMonkey on this too. If you guys do land this now, please be prepared for possibly having to adjust should the data structure change and you guys want to not diverge.

@jonathanKingston
Copy link

@jonathanKingston jonathanKingston commented Feb 15, 2015

@nzakas I had assumed so, just thought it was worth checking.

I had mostly got an Ember-cli app linted this morning using this code; I don't think there are any other hurdles in getting ESLint as a plugin there.

@caridy
Copy link
Contributor Author

@caridy caridy commented Feb 15, 2015

@nzakas I'm confident that I can get it ready next week, since we made good progress on estree/estree#11 already, once we get the thumbs up there, I will get this one done.

@nzakas
Copy link
Member

@nzakas nzakas commented Feb 19, 2015

@max-mykhailenko you can't ignore it, as far as ESLint is concerned, it's invalid syntax.

@caridy caridy force-pushed the caridy:modules branch from 0247083 to 249489c Feb 20, 2015
@caridy
Copy link
Contributor Author

@caridy caridy commented Feb 20, 2015

I get a chance to work on this PR this morning, here is the update:

  • use throwErrorTolerant instead of throwError
  • splitting export and import tests into separate subdirectories
  • remove the nasty rewind() hack in favor of jquery/esprima#310
  • sourceType="module" enables all ES6 (supported) features
  • settle with the spec defined in estree/estree#11 (esprima meeting is next wed.)
  • ModuleSpecifier is now just a literal: estree/estree#35 (comment)
@nzakas
Copy link
Member

@nzakas nzakas commented Feb 20, 2015

Thanks @caridy

@caridy
Copy link
Contributor Author

@caridy caridy commented Feb 20, 2015

@nzakas I will be on the road next week again, and I doubt that I will get a chance to work on this. We can either merge what we have (or a PR on my branch will be welcome if someone is up for helping with the two remaining small details from the list above). As for the refactor on the AST, that will take a little bit more time, until we settle on the discussions around that topic (hopefully after next wed meetings with jquery folks).

@nzakas
Copy link
Member

@nzakas nzakas commented Feb 21, 2015

That's okay. I'd rather hold off and get it right than need to break things later.

@xjamundx
Copy link
Contributor

@xjamundx xjamundx commented Mar 3, 2015

Anyway I can help?

@jonathanKingston
Copy link

@jonathanKingston jonathanKingston commented Mar 5, 2015

@caridy Looks like both blocking issues are resolved:
estree/estree#11 and estree/estree#35

Looks like little is blocking this from going live, is there anything else needs looking over or help with?

@nzakas is there an issue for ESLint how this will be implemented ticket yet?

@caridy caridy force-pushed the caridy:modules branch from 249489c to 055f201 Mar 5, 2015
@caridy
Copy link
Contributor Author

@caridy caridy commented Mar 5, 2015

Ok, we finally agree on the work on espree for Import/Export definition, and I just propagated the changes to espree, enjoy :)

UPDATE: I will resolve the conflicts for all the new stuff

@xjamundx and @jonathanKingston, if you guys have some time, we have 3 remaining things that we can do in other PRs, feel free to contribute:

  • add test to validate that sourceType="module" effectible enables strict mode (e.g.: WithStatement is invalid in modules)
  • ImportDeclaration and ExportDeclaration cannot be used inside functions (missing tests and implementation)
  • Enable classes for export and export default declarations (all TODOs are in the code, very straight forward)
@mikesherov
Copy link

@mikesherov mikesherov commented Mar 5, 2015

Hey @caridy, not to hijack here, but care to implement same PR to Esprima? Would be a shame to not have this upstreamed.

@caridy
Copy link
Contributor Author

@caridy caridy commented Mar 5, 2015

@nzakas solved the conflicts, ready to roll.
@mikesherov yeah, will try to do so tomorrow if I get a chance.

@mikesherov
Copy link

@mikesherov mikesherov commented Mar 5, 2015

Thanks!

@xjamundx
Copy link
Contributor

@xjamundx xjamundx commented Mar 5, 2015

@caridy I'll take ImportDeclaration and ExportDeclaration cannot be used inside functions (missing tests and implementation) for now and send the PR to caridy:modules Hopefully will be able to start tomorrow

caridy#1 - ImportDeclaration and ExportDeclaration in functions
caridy#2 - Enable class parsing for export
caridy#3 - strict mode in modules test

var expected = require(path.resolve(__dirname, "../../", MODULES_IMPORT_DIR, filename) + ".result.js");
var result;

try {

This comment has been minimized.

@xjamundx

xjamundx Mar 5, 2015
Contributor

Overall there is a lot of similar/duplicate code with what's in ecma-features.js already. Is there anyway we can combine them?

This comment has been minimized.

@caridy

caridy Mar 5, 2015
Author Contributor

probably outside of the scope of this PR.

This comment has been minimized.

@xjamundx

xjamundx Mar 5, 2015
Contributor

But isn't it a new file that was created in this PR :-p

Either way that's fine. I just got confused trying to add some debugging into ecma-feature.js when really it was needed in modules.js :)

This comment has been minimized.

@caridy

caridy Mar 5, 2015
Author Contributor

no, that's probably the merge after conflicts since this PR has been around for a while. These is the actual work: caridy@055f201

@nzakas
Copy link
Member

@nzakas nzakas commented Mar 5, 2015

Rather than forcing more work on Caridy for this, I'd like to merge into Espree as-is and work on making additional changes as enhancements. Any objections?

@caridy
Copy link
Contributor Author

@caridy caridy commented Mar 5, 2015

😌

@xjamundx
Copy link
Contributor

@xjamundx xjamundx commented Mar 5, 2015

@nzakas yes please! Happy to repoint the few extra test PRs back here once this merges

nzakas added a commit that referenced this pull request Mar 5, 2015
fixes #35: adds module import and export grammar
@nzakas nzakas merged commit 7d4a819 into eslint:master Mar 5, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nzakas
Copy link
Member

@nzakas nzakas commented Mar 5, 2015

Done, thanks @caridy. Moved the discussion to here: #72

@mikesherov
Copy link

@mikesherov mikesherov commented Mar 10, 2015

@caridy any problems with me re-porting this back to upstream Esprima?

@koutsenko
Copy link

@koutsenko koutsenko commented Mar 15, 2018

@caridy , hello :)

Can i do bulk named exports?
For example:

/* eslint-disable no-multi-spaces */

export * as AppStates       from './AppStates';
export * as Common          from './Common';
export * as ActionTypes     from './ActionTypes';
export * as ElementStates   from './ElementStates';
export * as MediaSyncStates from './MediaSyncStates';
export * as Urls            from './Urls';

It works, but eslint showing errors.

@deepsweet
Copy link

@deepsweet deepsweet commented Mar 15, 2018

@koutsenko this feature is still in a stage 1, so far you can use only export { default as Blah } from 'blah' as "stable".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet