-
Notifications
You must be signed in to change notification settings - Fork 30k
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 "src: simplify format in node_file.cc" #34463
Conversation
This reverts commit 673f49a. This change is breaking gulp on v14.x. Reverting so we can reland in a way that doesn't break stuff Refs: nodejs#33660 Refs: nodejs#34371
@himself65 FYI |
Can we please fast track so we can get this in 14.6.0 |
… this is very surprising – it’s purely a formatting change. If this breaks gulp, then gulp is already broken, but it seems almost impossible that this change actually affects behavior in anyway. If you are certain that this is breaking, maybe just leave the commit out of the v14.6.0 proposal? |
@addaleax this change went out in v14.5.0 and broke the gulp test suite. Reverting this change in v14.6.0 fixes it. I'll admit that I am also surprised that this change is having that effect. Although honestly I just ran the test suite again and this revert was not fixing it... TBH I'm very confused right now and will dig in a bit more |
@MylesBorins Maybe some other change made the test suite flaky? I think that seems like a more likely cause… |
there is definitely a change on 14.5.0 that broke gulp, perhaps this was a false positive from bisect Running bisect again and a few more manual tests... since when I checked again this revert didn't actually fix the problem I was seeing |
Closing as manual testing again on local machine shows gulp still failing |
This reverts commit 673f49a.
This change is breaking gulp on v14.x. Reverting so we can reland in a
way that doesn't break stuff
Refs: #33660
Refs: #34371