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 "lib: add WeakRef and FinalizationRegistry to primordials" #38238

Closed
wants to merge 2 commits into from

Conversation

BethGriggs
Copy link
Member

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Apr 14, 2021
@BethGriggs
Copy link
Member Author

Looks like #38211 was dependent on this, so this revert is failing the linter run. I'll see how the other runs go, but I'm guessing we'd potentially need to revert both to get back to passing runs.

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Apr 14, 2021

Even if this makes the run green I'm not convinced it's the right move. The reverted PR did not introduce the bug, it made it more visible.

@BethGriggs
Copy link
Member Author

@targos, the motivation for this PR was that when the actions are not green across the board, it raises the chance of other failures slipping through (and it appears PR #37263 landed with the failures).

Do you have an alternative suggestion? I skimmed through the linked issues, and #38000 did not appear to be imminently ready to land.

@targos
Copy link
Member

targos commented Apr 14, 2021

The alternative is to skip or mark the test flaky

@jasnell
Copy link
Member

jasnell commented Apr 14, 2021

If the test is failing consistently on every run then it's not flaky, it's just broken. My preference would be to go ahead and revert so that we can make sure CI is green, fix the test, then re-land the reverted change.

@targos
Copy link
Member

targos commented Apr 14, 2021

If something should be reverted, it's the change that broke the test in the first place, not another unrelated change that happens to make it red in CI. That test has been failing on my machine for several days already

@aduh95
Copy link
Contributor

aduh95 commented Apr 14, 2021

Looks like #38211 was dependent on this, so this revert is failing the linter run. I'll see how the other runs go, but I'm guessing we'd potentially need to revert both to get back to passing runs.

Actually it's not dependent, you only need to use globalThis from primordials to fix the linter issue. I can take care of this if you want.

@BethGriggs BethGriggs closed this Apr 14, 2021
@BethGriggs
Copy link
Member Author

BethGriggs commented Apr 14, 2021

Actually it's not dependent, you only need to use globalThis from primordials to fix the linter issue. I can take care of this if you want.

I meant dependent in the sense that I couldn't just revert 78343bb alone. I'll leave fixing the actions to folks who are more familiar with errors.

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

Successfully merging this pull request may close these issues.

5 participants