Parsing dependency JavaScript in strict mode

[christianbundy]

All of our dependencies work great, but it would be nice if we could parse them in strict mode. Around 98% of the files in node_modules can be parsed in JavaScript strict mode, which results in an interpretation that's simpler and more predictable for build tools, static analyzers, and experimental interpreters. I'm opening pull requests for the few files that don't work in strict mode, which is usually something small or silly like using octal instead of hexadecimal or accidentally defining a function twice.

I think strict mode is an easy win, but I'm open to suggestions on how this can be done better.

Here's what I'm using to find code that's invalid in strict mode:

eslint \
  --ignore-pattern '!**/*' \
  --no-eslintrc \
  --no-inline-config \
  --parser-options \
    '{ ecmaVersion: 8, sourceType: "module" }' \
  --rule '' \
  .

Solved (e.g. in a PR)

• [x] bash-color/color.js
• [x] bash-color/index.js
• [x] bl/test/test.js
• [x] chloride/small.js
• [x] compatibility/pretest.js
• [x] compatibility/test.js
• [x] flumedb/test/memlog.js
• [x] has-symbols/test/shams/core-js.js
• [x] has-symbols/test/shams/get-own-property-symbols.js
• [x] hexpp/bin.js
• [x] ltgt/index.js
• [x] map-filter-reduce/util.js
• [x] mkdirp/bin/cmd.js
• [x] multiserver/plugins/unix-socket.js
• [x] muxrpcli/index.js
• [x] object-inspect/test/lowbyte.js
• [x] obv/first.js
• [x] on-change-network/index.js
• [x] pull-box-stream/test/index.js
• [x] pull-file/test/terminate-read.js
• [x] pull-reader/example.js
• [x] pull-write-file/index.js
• [x] push-stream-to-pull-stream/index.js
• [x] push-stream/sources/values.js
• [x] push-stream/throughs/flatten.js
• [x] secret-handshake/test/secret-handshake.js
• [x] ssb-config/tests/server-startup.test.js
• [x] strip-indent/cli.js

Deployed (i.e. in ssb-server@master)

• [ ] bash-color/color.js
• [ ] bash-color/index.js
• [ ] bl/test/test.js
• [ ] chloride/small.js
• [ ] compatibility/pretest.js
• [ ] compatibility/test.js
• [x] flumedb/test/memlog.js
• [ ] has-symbols/test/shams/core-js.js
• [ ] has-symbols/test/shams/get-own-property-symbols.js
• [ ] hexpp/bin.js
• [ ] ltgt/index.js
• [ ] map-filter-reduce/util.js
• [ ] mkdirp/bin/cmd.js
• [ ] multiserver/plugins/unix-socket.js
• [x] muxrpcli/index.js
• [ ] object-inspect/test/lowbyte.js
• [ ] obv/first.js
• [ ] on-change-network/index.js
• [ ] pull-box-stream/test/index.js
• [ ] pull-file/test/terminate-read.js
• [ ] pull-reader/example.js
• [ ] pull-write-file/index.js
• [ ] push-stream-to-pull-stream/index.js
• [ ] push-stream/sources/values.js
• [ ] push-stream/throughs/flatten.js
• [ ] secret-handshake/test/secret-handshake.js
• [ ] ssb-config/tests/server-startup.test.js
• [ ] strip-indent/cli.js

[ljharb]

When you say “strict mode”, do you mean node’s strict flag, which isn’t the same as the language’s strict mode and should never be used?

[christianbundy]

Hello! I mean strict mode, which is implicit when working with EcmaScript modules (whereas scripts have to explicitly add "use strict"). Some transpilers, static analysis tools, and JavaScript runtimes parse all JS as modules, so being able to parse a module in strict mode makes it more accessible and predictable in different environments. For example, rollup suggests:

ES modules are always parsed in strict mode. That means that certain non-strict constructs (like octal literals) will be treated as syntax errors when Rollup parses modules that use them. Some older CommonJS modules depend on those constructs, and if you depend on them your bundle will blow up. There's basically nothing we can do about that.

Luckily, there is absolutely no good reason not to use strict mode for everything — so the solution to this problem is to lobby the authors of those modules to update them.

I don't really dig the "lobby the authors" approach, so I'm just writing patches and contributing them. Happy to chat about this more if you have questions or suggestions.

[ljharb]

Things that were written as Scripts should never be parsed as Modules, and things that were written for CJS the same. I’m not sure what the point is here (note that rollup often violates the spec for expediency in various ways, and this might be one of them)

[ljharb]

Separately, the language allows module level return in CJS in strict mode because CHS isn’t a Script; it’s wrapped in a function.

[christianbundy]

I think we're both well-informed about the difference between scripts and modules, maybe we could discuss the pros and cons? It seems beneficial to me that we could parse CJS modules as ES modules, and using strict mode in general seems to make code more predictable and easier to grok (especially when writing parsers / interpreters). The cons seem to be that modules would be restricted from using top-level returns, octal numbers, plus a handful of "sloppy mode" practices that I don't think anyone is doing. Is your position that top-level returns are more important than CJS <=> ES interoperability, or do I have a blind spot about some of the problems this might create?

Aside: I'm getting the feeling that you're unhappy with me, especially with the "you also indented things wrong" thing after rejecting and closing my PR (sorry, my auto-indent uses spaces unless you have .editorconfig/etc). Please let me know if there's some context I'm missing or if I said something that you found insulting / offensive. My goal was to volunteer a few hours improving compatibility and interoperability, so I wasn't really expecting the style of communication you've been using.

[ljharb]

In general, PRs opened on multiple dependencies with minimal explanation or justification, that imply that the code is “wrong” when in fact it works just fine, is hardly a courteous start.

Separately, applying any autoformatting to code is inappropriate unless such formatting is enforced by that project’s CI - when it is not, the expectation is that you manually match the project’s style.

As for the pros and cons of strict vs sloppy mode, or Scripts vs Modules, i trust we’re both informed - but none of that is relevant here. Code should only ever be parsed in the parsing goal the author intended, and it is always wrong and often fraught with bugs to do otherwise. Code authored in sloppy mode occasionally needs to be, especially in tests, which were the two PRs that led me here. this isn’t about interoperability - which esm and cjs already have, trivially - this is about attempting to parse code incorrectly and hoping it will work anyways.

Finally, the PRs themselves weren’t even needed since i assume you’re not bundling or running those deps’ tests.

I’m sorry that my communication style has been off putting; that wasn’t my intention.

[christianbundy]

Preamble: I don't really expect you to merge any of my PRs, so at this point I'm just trying to connect a bit more and understand your position a bit further. Please don't take any of the following as a request to re-open or merge any of my patches. :rocket:

In general, PRs opened on multiple dependencies with minimal explanation or justification, that imply that the code is “wrong” when in fact it works just fine, is hardly a courteous start.

I'm sorry about that, I didn't mean to imply that anything was "wrong". I could have framed the strict mode conformance as a feature rather than a "fix". Do you think that would be helpful in the future, or are there other changes you'd recommend?

Separately, applying any autoformatting to code is inappropriate unless such formatting is enforced by that project’s CI - when it is not, the expectation is that you manually match the project’s style.

I hit > and Vim indented with spaces (my default) and I didn't realize you used tabs (your default). I want to be clear that this wasn't the result of standard --fix or anything similar, but I also understand that having weird indentation added to your project is a pain.

Code authored in sloppy mode occasionally needs to be, especially in tests, which were the two PRs that led me here.

Could you maybe explain what those patches broke, or what the downside is? Modules that conform to strict mode are simpler for machines to analyze and transform, so it feels like a super easy win to use hexadecimal instead of octal, but since you rejected the PR it sounds like there's a downside I'm not aware of. You are of course under no obligation to accept any pull requests, I'm just looking to build a bit of a bridge and understand why you wouldn't want a one-line fix that makes the entire module conform to strict mode.

Finally, the PRs themselves weren’t even needed since i assume you’re not bundling or running those deps’ tests.

You're right, but if I wanted to run your tests in something like Secure EcmaScript or do some static analysis then there are a handful of files my parser would reject. If migrating to strict mode took hours or days then I'd probably think it was silly, but out of 13,000 .js files in node_modules only 30 files needed tiny fixes to avoid octals, global returns, duplicate variables, and all sorts of other small things that the non-strict parser allows. My understanding is that ES modules are becoming the de facto standard, so I thought conforming to strict mode would be considered an improvement rather than me stomping all over your garden.

I’m sorry that my communication style has been off putting; that wasn’t my intention.

I'm sorry too, I understand that this issue probably could've had a lot more info and context in the body, plus it would have probably been nice to frame this as a feature rather than a bugfix. Thanks for iterating on this discussion with me, sometimes text communication is hard. :mega:

[ljharb]

One thing that i noticed in particular is that these PRs didn’t add the use strict pragma - that would have made the code actively break without the resulting fixes, making those fixes necessary but not the focus of review. Without the pragma, there’s also nothing to prevent the changes from regressing.

When a repo has eslint, eslint settings can be applied as well to enforce this - and in some of my packages, the config explicitly allows sloppy mode in tests for the purposes of testing what happens in sloppy mode (for example).

I don’t have specific examples of what your PRs break, nor even certainty that they do break - but lacking a regression prevention mechanism is a red flag to be sure.

[christianbundy]

Oh, I was actually trying to avoid adding "use strict" or configuring ESLint because I thought that would be more heavy-handed. I'd be happy to do either of those if that's your preference.

[dominictarr]

@christianbundy you've just caused me to be sent a bunch of emails, creating work for me, but from my perspective it doesn't fix anything. The code already works. It doesn't make any difference to me if it's strict or not. (doesn't fix a bug, etc) if this is important to you, please just merge and publish all the stuff you have access to, but I'm just gonna ignore this. I agree with @ljharb about "use strict" and regressions

[christianbundy]

It doesn't make any difference to me if it's strict or not.

Sorry, I thought it was something you'd dig. From my perspective these changes made our code easier to parse programatically and gives us the option of using strict mode (silent errors become loud, code can get faster, and syntax is more future-proof). Sorry to waste your time.

[dominictarr]

code can get faster

"faster" is something you can measure, I would be impressed if you could measure a performance impact of making everything not already strict strict (given that the things which need optimizing probably already are)