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

Revert "fs: remove workaround for esm package" #51356

Closed

Conversation

jeremymeng
Copy link

This reverts commit 95b1989.

It appears that the workaround is still needed per issue #51081

aduh95 and others added 30 commits November 25, 2023 16:32
Sometimes reporters link to a repo for their repro, which
not only likely takes them more time to setup, but also is less
convenient for maintainers. Setting up a repo goes against the idea of a
minimal repro, as if it was actually minimal, they would not have
bothered creating a repo.

PR-URL: nodejs#50882
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Every other invocation of BN_bn2binpad checks the return value. For
safety and consistency, do so in RandomPrimeTraits::EncodeOutput()
as well.

PR-URL: nodejs#50860
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#50881
Fixes: nodejs#50875
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Increase the number of iterations from 1e5 to 5e6
to avoid the test performance gap caused by inactive
V8 optimization caused by insufficient number of iterations

Refs: nodejs#50571
PR-URL: nodejs#50698
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
PR-URL: nodejs#50834
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
PR-URL: nodejs#49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
PR-URL: nodejs#49884
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Since website based on 2 different repos, there was an inconsistency
in theme selection, so we had 2 independant theme props.
Now only one stored in local storage is a single source of truth

PR-URL: nodejs#50877
Reviewed-By: Claudio Wunder <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
fix pr url

PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#50488
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Theme applying logic get loaded and executed in async mode, so often
there is a
delay in applying the proper theme to a page which leads to flicker on
dark theme. Resolved by moving critical logic to the page head

PR-URL: nodejs#50942
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Claudio Wunder <[email protected]>
PR-URL: nodejs#50907
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
PR-URL: nodejs#50904
Refs: nodejs#49667
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
We previously used a text that appears to be an excerpt of
https://zh.wikipedia.org/wiki/%E5%8D%97%E8%B6%8A%E5%9B%BD
and can have copyright/license complications. It may
also include some geopolitical nuances. The text has been
repeated through out the code base without much reuse.

This patch consolidates the fixtures by adding a common helper
string as `fixtures.utf8TestText` which is identical to a copy
in test/fixtures/utf8_test_text.txt. It also updates the text
to a copy of 蘭亭集序, It was chosen because:

1. It's a well-known Chinese classical piece written in 353 CE
   and therefore in public domain. The string is copied from
   https://zh.wikisource.org/zh-hant/%E8%98%AD%E4%BA%AD%E9%9B%86%E5%BA%8F
   which contains a disclaimer of copyright for this reason.
2. The text is in suitable length for general UTF8 string
   read/write tests (including punctuations, 389 code points and
   1167 bytes).
3. This is also commonly used as reference text for Chinese text
   layout tests.
4. It's a timeless and harmless preface for a collection of poems,
   written by a uncontroversial figure who passed away >1600 years
   ago and contains no geopolitical nuances. Background and an
   English translation of this text can be found at
   https://en.wikipedia.org/wiki/Lantingji_Xu

PR-URL: nodejs#50732
Reviewed-By: Yagiz Nizipli <[email protected]>
cola119 and others added 21 commits December 29, 2023 13:10
PR-URL: nodejs#51293
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#51293
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#51293
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#51293
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#51293
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#51293
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Replace directories instead of just copying to take removed files into
account.

PR-URL: nodejs#51294
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs#51291
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
PR-URL: nodejs#51291
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
PR-URL: nodejs#51296
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
PR-URL: nodejs#50790
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Replace `Array.prototype.forEach()` with `for...of` in
`test/parallel/test-fs-readv-sync.js` and
`test/parallel/test-fs-readv.js`.

PR-URL: nodejs#50787
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Replace `Array.prototype.forEach()` with `for...of` in
`parallel/test-whatwg-encoding-custom-textdecoder-utf16-surrogates.js`.

PR-URL: nodejs#50608
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Introduce HasDoTryWrite and make use of it in WriteString

PR-URL: nodejs#51155
Reviewed-By: Luigi Pinca <[email protected]>
Co-Authored-By: Carlos Espa <[email protected]>
PR-URL: nodejs#50758
Refs: nodejs/security-wg#898
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Claudio Wunder <[email protected]>
Add a comment to clarify that the `fileName` parameter can be `null` if
the file name cannot be determined.

PR-URL: nodejs#51305
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs#51271
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#51317
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#51318
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#51320
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
This reverts commit 95b1989.

It appears that the workaround is still needed: issue nodejs#51081
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jan 4, 2024
@aduh95
Copy link
Contributor

aduh95 commented Jan 4, 2024

Not sure about this, esm was never patched, and the repo is now archived which makes me think it will never be patched. I'm not sure we would want to revert. That being said, I think the other PR should have been marked as semver-major PRs that contain breaking changes and should be released in the next major version. , and not land in the 21.x release line. So I think it would make more sense if this targets the 21.x-staging branch instead. wdyt @nodejs/releasers @nodejs/tsc ?

@RafaelGSS
Copy link
Member

Agree.

@mcollina
Copy link
Member

mcollina commented Jan 4, 2024

agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.