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

fails in node --strict_mode #9

Closed
bcoe opened this issue Apr 10, 2016 · 8 comments
Closed

fails in node --strict_mode #9

bcoe opened this issue Apr 10, 2016 · 8 comments
Assignees
Labels

Comments

@bcoe
Copy link

bcoe commented Apr 10, 2016

when you run a module that depends on spdx-expression-parse.js in strict mode, things explode:

node --use_strict /Users/benjamincoe/bcoe/yargs/node_modules/read-pkg-up/node_modules/read-pkg/node_modules/normalize-package-data/node_modules/validate-npm-package-license/node_modules/spdx-expression-parse/parser.generated.js 

/Users/benjamincoe/bcoe/yargs/node_modules/read-pkg-up/node_modules/read-pkg/node_modules/normalize-package-data/node_modules/validate-npm-package-license/node_modules/spdx-expression-parse/parser.generated.js:1
k.length=vstack.length-n;lstack.length=lstack.length-n}_token_stack:function l
                                                                    ^^^^^^^^
SyntaxError: In strict mode code, functions can only be declared at top level or immediately within another function.
    at Module._compile (module.js:439:25)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:929:3

This issue effects a cascading chain of modules, it was actually brought up to me on the yargs repo:

https://www.npmjs.com/package/yargs

But it also effects @sindresorhus' popular:

https://www.npmjs.com/package/read-pkg-up

And the popular npm utility module:

https://www.npmjs.com/package/normalize-package-data

I tried reproducing the issue, and simply running newer versions of the parser used for spdx-expression-parse seems to help solve this issue -- it might just be a matter of rebuilding and publishing a patch release.

@sindresorhus
Copy link

Ugh, node --use_strict is such an anti-pattern. Nobody should be using that.

nodejs/node-v0.x-archive#7479
https://github.com/substack/node-mkdirp/issues/32

@bcoe
Copy link
Author

bcoe commented Apr 10, 2016

@sindresorhus I don't know much about --use_strict, except for the issue being brought up on yargs.

It it worth publishing a point release, since it seems it's just a matter of rebuilding -- or should I just take a hard-line on this?

@sindresorhus
Copy link

It it worth publishing a point release, since it seems it's just a matter of rebuilding -- or should I just take a hard-line on this?

That's really up to you, but accepting this is a slippery slope. I would not. There are thousands of modules on npm that would fail in strict mode. There's no good reason to use that flag.

@kemitchell
Copy link
Member

I've managed to reproduce in Travis build 52.

I was not aware of --use_strict. I've enjoyed learning about it about as much as I enjoy learning about new unpleasant, fatal diseases. Perhaps I'm more world-savvy for it, but if someone had asked, I'd've kept my bliss.

I'm in sympathy with whoever has used the flag and suffered its predictable results. But I'm with @sindresorhus that discouragement is the only proper prescription here, as much for their sake as anyone else's. I wish they'd come direct to me, rather than to the more popular yargs, since poor @bcoe has enough problems, and now it has to bubble its way back up again.

A patch release isn't usually much to ask, but in this case we're talking about a parser generator. It's a parser generator I love, whose contributors I respect, but I've also had some issues with it in the past. I have tests, but I wouldn't publish on green here. Not with npm CLI downstream.

@kemitchell kemitchell self-assigned this Apr 10, 2016
@bcoe
Copy link
Author

bcoe commented Apr 11, 2016

@kemitchell I'm happy to take a hard-line on this, if Sindresorhus and Substack are fine with closing --use_strict pulls, I'm happy to as well.

@Alphapage
Copy link

I'm sorry, but I think strict mode is the new standard.
http://www.ecma-international.org/ecma-262/6.0/#sec-strict-mode-code
If your tools are not compatible with es2015 modules or classes which are always in strict mode, then those tools become obsolete and unusable.
I think you should investigate in supporting strict mode quickly, but this is just my opinion.

@kemitchell
Copy link
Member

@Alphapage, strict mode is part of newer editions of ECMAScript, but as an opt-in feature triggered by the 'use strict' directive. ECMA does not specify global interpreter behavior, like --use_strict, that imposes strict mode on code whether it has the directive or not.

@Alphapage
Copy link

As far as I can understand from my previous link::

  • Module code is always strict mode code.
  • All parts of a ClassDeclaration or a ClassExpression are strict mode code.

It is impossible to use it inside es6 class or import because an error will be thrown.

motet-a added a commit to motet-a/spdx-expression-parse.js that referenced this issue Jun 15, 2017
This subjective and opinionated patch fixes many issues but has
several drawbacks.

The parser should be usable in strict mode now (see jslicense#9). It is not
longer generated by `jison`. `parse.js` contains a simple handwritten
recursive decent parser. It should be easier to debug and to hack, but
there is definitely more code and one can prefer parser generators.
Note that `jison` seems to have big maintainance issues.

Note that [Yargs](https://github.com/yargs/yargs) is not usable in strict mode because it depends on
this package.

I also simplified the lexer (IMHO).

I wrote more Mocha-based tests in `test/index.js`. I really like the
embedded tests in `README.md`, but I was afraid to bloat it with
boring spec-compliance tests.

This patch does not introduce a breaking change and does not change
dependencies (however, it changes devDependencies).
motet-a added a commit to motet-a/spdx-expression-parse.js that referenced this issue Jun 15, 2017
This subjective and opinionated patch fixes many issues but has
several drawbacks.

The parser should be usable in strict mode now (see jslicense#9). It is not
longer generated by `jison`. `parse.js` contains a simple handwritten
recursive decent parser. It should be easier to debug and to hack, but
there is definitely more code and one can prefer parser generators.
Note that `jison` seems to have big maintainance issues.

Note that [Yargs](https://github.com/yargs/yargs) is not usable in strict mode because it depends on
this package.

I also simplified the lexer (IMHO).

I wrote more Mocha-based tests in `test/index.js`. I really like the
embedded tests in `README.md`, but I was afraid to bloat it with
boring spec-compliance tests.

This patch does not introduce a breaking change and does not change
dependencies (however, it changes devDependencies).
kemitchell pushed a commit that referenced this issue Jun 15, 2017
This subjective and opinionated patch fixes many issues but has
several drawbacks.

The parser should be usable in strict mode now (see #9). It is not
longer generated by `jison`. `parse.js` contains a simple handwritten
recursive decent parser. It should be easier to debug and to hack, but
there is definitely more code and one can prefer parser generators.
Note that `jison` seems to have big maintainance issues.

Note that [Yargs](https://github.com/yargs/yargs) is not usable in strict mode because it depends on
this package.

I also simplified the lexer (IMHO).

I wrote more Mocha-based tests in `test/index.js`. I really like the
embedded tests in `README.md`, but I was afraid to bloat it with
boring spec-compliance tests.

This patch does not introduce a breaking change and does not change
dependencies (however, it changes devDependencies).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants