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

v2.0.0-alpha-2: Support for ES3/5 'modules' (implicit strict mode) is lost #4832

Closed
nre opened this issue Dec 30, 2015 · 18 comments

Comments

Projects
None yet
6 participants
@nre
Copy link
Contributor

commented Dec 30, 2015

ecmaFeatures.modules = true in v1.10.3 supports ES3/5 'modules' as well as ES6 modules, whereas parserOptions.sourceType = "module" in v2.0.0-alpha-2 only supports ES6 modules (because it silently overrides parserOptions.ecmaVersion: https://github.com/eslint/eslint/blob/master/lib/eslint.js#L418).

An ES3/5 'module' may be implicitly in strict mode. For example, this file:

// myModule.js
exports.foo = "bar";

is implicitly in strict mode because it will be bundled up into something like this:

// bundle.js
loadModules({
    myModule: function (require, exports, module) {
        "use strict";
        // myModule.js
        exports.foo = "bar";
    },
});

Support for ES3/5 'modules' and implicit strict mode can be tested with the following files, both of which contain an error:

// es6.js
const ES6 = true;
// invalidThis.js
function invalidThis() {
    return this;
}

v1.10.3 supports ES3/5 'modules' because it finds both errors:

{
    "ecmaFeatures": {
        "modules": true
    },
    "rules": {
        "no-invalid-this": 2
    }
}
es6.js
  2:2  error  Parsing error: Unexpected token const

invalidThis.js
  3:9  error  Unexpected `this`  no-invalid-this

✖ 2 problems (2 errors, 0 warnings)

v2.0.0-alpha-2 does not support ES3/5 'modules' because it can find either the first error:

{
    "parserOptions": {
        "ecmaVersion": 5
    },
    "rules": {
        "no-invalid-this": 2
    }
}
es6.js
  2:2  error  Parsing error: The keyword 'const' is reserved

✖ 1 problem (1 error, 0 warnings)

or the second error (but not both):

{
    "parserOptions": {
        "ecmaVersion": 5,
        "sourceType": "module"
    },
    "rules": {
        "no-invalid-this": 2
    }
}
invalidThis.js
  3:9  error  Unexpected `this`  no-invalid-this

✖ 1 problem (1 error, 0 warnings)

Although it is correct that parserOptions.sourceType = "module" should imply ES6 (although perhaps not silently), I hope that support for ES3/5 'modules' and implicit strict mode can be restored to v2 in some way.

@nre

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2015

I had assumed that all JS bundlers started each module with "use strict" by default, but apparently some people are still running/writing non-strict-mode JS!

Here is a list of tools to show that I'm not the only one who writes their ES3/5 'modules' with implicit strict mode, for browsers as well as Node:

@nzakas nzakas removed the bug label Dec 31, 2015

@nzakas

This comment has been minimized.

Copy link
Member

commented Dec 31, 2015

Using ES6 modules as a proxy to say "we have implicit strict mode" is not a good solution because modules are not simply implicit strict mode, they imply a lot of different things.

I'm very unfamiliar with this use case. Are you saying some custom module systems insert strict mode when compiling?

@michaelficarra

This comment has been minimized.

Copy link
Member

commented Dec 31, 2015

@nzakas The only things I can think of:

  1. Strict
  2. Allow import/export statements
  3. Top level declarations are not globals
  4. async is a keyword
@nzakas

This comment has been minimized.

Copy link
Member

commented Dec 31, 2015

await is reserved as well, correct?

@michaelficarra

This comment has been minimized.

Copy link
Member

commented Dec 31, 2015

Oops, that's what I meant. Not async.

@nre

This comment has been minimized.

Copy link
Contributor Author

commented Dec 31, 2015

The use case is laziness. "use strict" in every module seems redundant when I only ever read, write and run strict-mode JS. And the job of build tools is to do these repetitive tasks!

I understand now that the old ecmaFeatures.modules option referred to ES6 modules. But the docs said 'enable modules and global strict mode' and I took this to mean what I was already doing: CommonJS modules and implicit strict mode.

Looking at 1.10.3, I see that setting ecmaFeatures.modules also set ecmaVersion to 6. But ecmaVersion was only passed to escope, not to the parser or anyone else. The result was like ES5 CommonJS modules and implicit strict mode.

I've looked for a way to give eslint an 'implicit strict mode' option, but I think it isn't possible. I can see how to fool escope, but unfortunately not the parser. If anyone knows a way then I'd be grateful to hear it.

Otherwise this non-issue can be closed.

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 1, 2016

It is possible to add an option, but it's a nontrivial amount of work as it would need to change Espree, escope, and ESLint together.

Do you have other options on your end?

@nre

This comment has been minimized.

Copy link
Contributor Author

commented Jan 1, 2016

My options are:

  1. Use v2 and get used to having "use strict"; in my modules. (Not a big deal.)
  2. Use v2 and have my ES3/5 modules linted as ES6. (Not great, but I know the difference between ES3/5 and ES6.)
  3. Stay on v1 until I start using ES6. (I'm waiting for good browser support first.)

JSHint has a closed issue for strict: implied (jshint/jshint#924). You can see that there's moderate demand, but not huge. The feature is supposed to be in v2.9 but that's still in pre-release, so it's not like users who migrate to eslint will be disappointed. (Although I was quite happy when I thought eslint did have the feature!)

I can fool escope without changing it:

var useStrictAst = espree.parse('"use strict";');
var useStrictStatement = useStrictAst.body[0];

ast.body.splice(0, 0, useStrictStatement);
var scopeManager = escope.analyze(ast, options);
ast.body.splice(0, 1);

Is this good enough for eslint?

The parser is difficult because it takes a string. A directive is ignored if it is added to the end of the string. Adding a directive to the start means all the start/end character offsets need to be fixed. (Does eslint use these, or only the loc objects? Would only the column numbers of line 1 need to be fixed?) I think the only clean solution is for parsers to expose their isStrict initial value.

I agree that this lazybones feature doesn't warrant non-trivial hacking.

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 2, 2016

Unfortunately that's not a feasible solution. You need to know that it's strict mode during the parsing phase to catch strict mode errors, just tricking escope doesn't result in catching these errors.

If someone wants to take a stab at this, I envision the steps to be:

  1. Add ecmaFeatures.impliedStrict to Espree
  2. Add a similar flag to escope
  3. Update ESLint to use them and update docs
@nre

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2016

TODO

@nre

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2016

@nzakas

The impliedStrict option has been merged in Escope, but I think it might be weeks/months until it is released. (I have requested a new release: estools/escope#98.) That is the only issue blocking the impliedStrict option in ESLint.

Would you reconsider permitting the AST-mutation Escope workaround? It would look like this:

/*
 * Temporary workaround until Escope supports the `impliedStrict`
 * option.
 */
shouldWorkAroundEscopeImpliedStrict = ecmaFeatures.impliedStrict;

if (shouldWorkAroundEscopeImpliedStrict) {
    impliedStrictAst = parse("'use strict';", {parser: config.parser});
    impliedStrictDirective = impliedStrictAst.body[0];
    ast.body.unshift(impliedStrictDirective);
}

// gather data that may be needed by the rules
scopeManager = escope.analyze(ast, {
    ignoreEval: true,
    nodejsScope: ecmaFeatures.globalReturn,
    // impliedStrict: ecmaFeatures.impliedStrict,
    ecmaVersion: ecmaVersion,
    sourceType: currentConfig.parserOptions.sourceType || "script"
});
currentScopes = scopeManager.scopes;

if (shouldWorkAroundEscopeImpliedStrict) {
    ast.body.shift();
}

I'm not asking this for myself (I'm using my patched versions of Escope and ESLint) but so that the feature is available to anyone who might want it. (JSHint has the feature in its latest stable release.)

@nzakas

This comment has been minimized.

Copy link
Member

commented Feb 1, 2016

Why would there be such a delay for escope?

@michaelficarra

This comment has been minimized.

Copy link
Member

commented Feb 1, 2016

@nre I've published escope 3.4.0.

@nzakas

This comment has been minimized.

Copy link
Member

commented Feb 1, 2016

Thanks @michaelficarra!

@nzakas nzakas added enhancement accepted and removed evaluating labels Feb 1, 2016

nre added a commit to nre/eslint that referenced this issue Feb 2, 2016

@nzakas nzakas closed this in 0a59926 Feb 3, 2016

nzakas added a commit that referenced this issue Feb 3, 2016

Merge pull request #5134 from nre/issue4832
Update: 'implied strict mode' ecmaFeature (fixes #4832)
@silvenon

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2016

@shaunstanislaus this is a long shot, but are you getting that error from AtomLinter/linter-eslint or ESLint itself?

@sgnl

This comment has been minimized.

Copy link

commented Apr 9, 2016

I was about to post that I'm experiencing this issue but after restarted ST3 about 4 times the error is gone... ¯_(ツ)_/¯

jrmlstfMt added a commit to jrmlstfMt/eslint-config-standard-deviation--es5 that referenced this issue Nov 30, 2016

Fix sourcetype in order to make it es5
When sourcetype is module ecmaversion 6 is implied.

See eslint/eslint#4832

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.