fix: avoid using bigint literal syntax #542
Conversation
|
Can you please add a test? |
|
@mcollina I'd love to but I'm not familiar with the code base that much and looking around I'm not sure what to test exactly, but if you give me some guidance or pointers I'll be happy to add one (or more). I see that tests in https://github.com/nodejs/readable-stream/blob/main/test/ours/test-errors.js basically only cover creating of specific instances of Error. How would you like the test to work? |
|
@mcollina ping. Could you please help me understand what tests would you like to see? I'd like to get this merged. Thx! |
|
My understanding is that this error is creating problems for a certain set of bundlers and/or browsers. Therefore, we need a test that verify we would not regress this in the future. Use said bundlers/browser and verify this. As you can see, we use playwright: https://github.com/nodejs/readable-stream/blob/main/test/browser/runner-browser.mjs. |
|
My understanding is that it does not actually cause issues in bundlers. The original code uses new JS syntax that gets passed through "as is" using any known bundler and/or transpiler (like babel) and so the resulting code fails to parse correctly if the target environment JS runtime is not able to handle the new JS syntax. So the point of the PR is to simply avoid the new JS syntax. That is all. I'm not sure how to test it 🤷♂️. |
|
Yeah, this issue concerns a runtime feature, which cannot go in a regular test case. Still, it is important to make the CI fail on this usage, so future code changes don't inadvertently use literals again. Ideally the test suite would be extended to test under a runtime with no BigInt support, but the seems complicated to implement. Alternatively, BigInt literal usage could be catched the linting stage, with a new eslint rule. Unfortunately there doesn't seem to be any existing rules that handle this. It should be simple enough to implement a custom rule though, as the logic is just these lines. |
|
Thanks @kanongil for your view. Agree completely. In my codebase I was actually able to workaround this problem by using rather simple babel plugin that replaces the bigint What do you suggest for a proper fix though? I will be happy to contribute a PR to create this new rule and configure it where appropriate if you give me pointers... |
|
We would at least want an ESLint rule that disallows BigInt literals. So basically take the code that @kanongil mentioned, wrap it a small local ESLint rule inside this project and add that rule to our I don't think we have tests for our lint rules. Just make sure that ESLint actually errors if you add a BigInt literal to the code. 😉 |
|
@mcollina @kanongil @MattiasBuelens PR description updated. Created a custom ESLint rule, turned it into a custom local ESLint plugin, referencing in a new style ESLint flat config file. Seems to work. What do you think? |
|
If CI is happy, it works for me! |
|
@mcollina when can this be merged? CI failures are because there are no way to install some node versions in MacOS: |
|
Could you skip those platforms? |
|
I'm not the author. I'm just interested in this getting merged. Maybe @mman can |
|
@mcollina Updated the workflow files to exclude node |
Attempts to address #541. After the discussion I have made a custom ESLint plugin that exposes
no-big-intrule to catch usage of the bigintnliteral as part ofnpm run lint. In order to actually make the local ESLint plugin / local ESLint rule work, I have to also migrate the ESLint configuration to the new flat file format.Running the
npm run lintagainstmainreports this (note that there are unrelated linting errors that should probably be addressed in a separate PR).Running it against my fork looks clean: