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

caridy · 2015-01-27T19:14:45Z

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)

nzakas · 2015-01-27T19:35:17Z

lib/features.js

+    jsx: false,
+
+    // enable parsing of import/export declarations
+    module: false


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

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.

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'.

nzakas · 2015-01-27T19:39:09Z

Overall looks great! Just a couple of notes.

And some bookkeeping stuff:

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

Thanks!

nzakas · 2015-01-27T19:40:18Z

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.

cvrebert · 2015-01-28T02:18:20Z

espree.js

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


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

xjamundx · 2015-01-28T18:46:49Z

doing cartwheels about this 🎠

nzakas · 2015-01-28T20:29:31Z

espree.js

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


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

ok, that makes more sense!

caridy · 2015-01-29T06:55:54Z

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

nzakas · 2015-01-29T16:39:00Z

Sure thing, thanks for the update.

vslinko · 2015-01-31T09:37:35Z

👍

caridy · 2015-02-06T17:29:04Z

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 · 2015-02-06T19:06:38Z

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

mikesherov · 2015-02-06T19:06:58Z

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

mikesherov · 2015-02-06T19:37:47Z

espree.js

+        // covers:
+        // export default ...
+        lex();
+        if (matchKeyword("function") || matchKeyword("class")) {


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

caridy · 2015-02-06T19:43:53Z

@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 · 2015-02-13T03:31:11Z

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 · 2015-02-13T03:44:50Z

@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 · 2015-02-13T04:10:51Z

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

cvrebert · 2015-02-13T05:03:52Z

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

nzakas · 2015-02-14T00:32:53Z

Fine by me.

jonathanKingston · 2015-02-14T13:47:58Z

@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 · 2015-02-14T23:16:01Z

@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 · 2015-02-14T23:29:59Z

@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 · 2015-02-15T00:42:11Z

@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 · 2015-02-15T01:07:24Z

@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.

caridy · 2015-02-20T17:45:18Z

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 Parsing export default ... peeks to not overconsume tokens jquery/esprima#310
sourceType="module" enables all ES6 (supported) features
settle with the spec defined in Import Specifiers: slightly inconsistent between Acorn and Esprima estree/estree#11 (esprima meeting is next wed.)
ModuleSpecifier is now just a literal: Add import nodes. estree/estree#35 (comment)

nzakas · 2015-02-20T23:29:24Z

Thanks @caridy

caridy · 2015-02-20T23:44:08Z

@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 · 2015-02-21T01:00:41Z

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

xjamundx · 2015-03-03T18:18:05Z

Anyway I can help?

jonathanKingston · 2015-03-05T01:57:41Z

@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 · 2015-03-05T02:20:18Z

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 · 2015-03-05T02:23:22Z

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

caridy · 2015-03-05T02:34:30Z

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

mikesherov · 2015-03-05T02:37:10Z

Thanks!

xjamundx · 2015-03-05T05:50:38Z

@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

xjamundx · 2015-03-05T15:04:40Z

tests/lib/modules.js

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


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

probably outside of the scope of this PR.

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 :)

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 · 2015-03-05T16:35:03Z

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 · 2015-03-05T16:38:46Z

😌

xjamundx · 2015-03-05T17:00:38Z

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

fixes #35: adds module import and export grammar

nzakas · 2015-03-05T18:57:09Z

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

mikesherov · 2015-03-10T01:44:09Z

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

koutsenko · 2018-03-15T09:26:42Z

@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 · 2018-03-15T12:29:02Z

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

eslint/js#43

nzakas reviewed Jan 27, 2015
View reviewed changes

cvrebert reviewed Jan 28, 2015
View reviewed changes

nzakas reviewed Jan 28, 2015
View reviewed changes

nzakas mentioned this pull request Jan 31, 2015

ES6 - Unexpected reserved keyword eslint/eslint#1688

Closed

xjamundx mentioned this pull request Feb 4, 2015

ES6 Rule Wishlist eslint/eslint#1617

Closed

mikesherov mentioned this pull request Feb 6, 2015

ES6 feature: Modules: Tracking Issue jquery/esprima#313

Closed

4 tasks

mikesherov reviewed Feb 6, 2015
View reviewed changes

sebmck mentioned this pull request Feb 13, 2015

Acorn: ImportBatchSpecifier vs Esprima: ImportNamespaceSpecifier estree/estree#9

Closed

matthewwithanm mentioned this pull request Feb 13, 2015

.eslintrc Proposal hzdg/linter-configs#1

Closed

caridy force-pushed the modules branch from 0247083 to 249489c Compare February 20, 2015 17:33

This was referenced Feb 27, 2015

Support ExportDeclaration in no-unused-vars rule eslint/eslint#1890

Closed

Add support for ExportDeclaration in no-unused-vars rule eslint/eslint#1903

Closed

New: Add ES6 module support (fixes eslint#35)

055f201

caridy force-pushed the modules branch from 249489c to 055f201 Compare March 5, 2015 02:13

merging upstream and solving conflicts for PR eslint#43

bb258cb

xjamundx reviewed Mar 5, 2015
View reviewed changes

nzakas added a commit that referenced this pull request Mar 5, 2015

Merge pull request #43 from caridy/modules

7d4a819

fixes #35: adds module import and export grammar

nzakas merged commit 7d4a819 into eslint:master Mar 5, 2015

caridy mentioned this pull request Mar 23, 2015

fixes 1000: support ES6 import and export declarations jquery/esprima#1149

Closed

UnsungHero97 mentioned this pull request Sep 21, 2016

Feature/auth0 login dataloom/gallery#3

Merged

sambacha added a commit to sambacha/typescript-cjs-esm-tests that referenced this pull request Jun 25, 2023

Add: ES6 exports / imports cheat sheet

1fbb1da

eslint/js#43

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

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

Conversation

caridy commented Jan 27, 2015

Export Syntax

Import Syntax

Other notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas commented Jan 27, 2015

nzakas commented Jan 27, 2015

Choose a reason for hiding this comment

xjamundx commented Jan 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caridy commented Jan 29, 2015

nzakas commented Jan 29, 2015

vslinko commented Jan 31, 2015

caridy commented Feb 6, 2015

mikesherov commented Feb 6, 2015

mikesherov commented Feb 6, 2015

Choose a reason for hiding this comment

caridy commented Feb 6, 2015

sebmck commented Feb 13, 2015

caridy commented Feb 13, 2015

nzakas commented Feb 13, 2015

cvrebert commented Feb 13, 2015

nzakas commented Feb 14, 2015

jonathanKingston commented Feb 14, 2015

nzakas commented Feb 14, 2015

mikesherov commented Feb 14, 2015

jonathanKingston commented Feb 15, 2015

caridy commented Feb 15, 2015

caridy commented Feb 20, 2015

nzakas commented Feb 20, 2015

caridy commented Feb 20, 2015

nzakas commented Feb 21, 2015

xjamundx commented Mar 3, 2015

jonathanKingston commented Mar 5, 2015

caridy commented Mar 5, 2015

mikesherov commented Mar 5, 2015

caridy commented Mar 5, 2015

mikesherov commented Mar 5, 2015

xjamundx commented Mar 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas commented Mar 5, 2015

caridy commented Mar 5, 2015

xjamundx commented Mar 5, 2015

nzakas commented Mar 5, 2015

mikesherov commented Mar 10, 2015

koutsenko commented Mar 15, 2018 • edited Loading

deepsweet commented Mar 15, 2018

koutsenko commented Mar 15, 2018 •

edited

Loading