-
-
Notifications
You must be signed in to change notification settings - Fork 165
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
Parsing dependency JavaScript in strict mode #683
Comments
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? |
Hello! I mean strict mode, which is implicit when working with EcmaScript modules (whereas scripts have to explicitly add
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. |
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) |
Separately, the language allows module level return in CJS in strict mode because CHS isn’t a Script; it’s wrapped in a function. |
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 |
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. |
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. 🚀
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?
I hit
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.
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
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. 📣 |
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. |
Oh, I was actually trying to avoid adding |
@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 |
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. |
"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) |
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:
Solved (e.g. in a PR)
Deployed (i.e. in
ssb-server@master
)The text was updated successfully, but these errors were encountered: