chore: bump deps, fix expected test result for core-js #4300
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4300 +/- ##
=======================================
Coverage 98.45% 98.45%
=======================================
Files 205 205
Lines 7311 7311
Branches 2082 2082
=======================================
Hits 7198 7198
Misses 55 55
Partials 58 58 Continue to review full report at Codecov.
|
dnalborczyk
force-pushed
the
bump-deps-core-js
branch
from
1932f44
to
4f0a409
Compare
|
@lukastaegert I noticed that 28cc198 and all commits starting from dbcf24a have been randomly failing the watch tests with timeouts on my Mac - irrespective of NodeJS version (v12.x, v14.x, v16.x): However, when I revert this single package and run $ git diff -U0
diff --git a/package-lock.json b/package-lock.json
index 116e472..6591429 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -484,3 +484,3 @@
- "version": "21.0.1",
- "resolved": "https://registry.npmjs.org/@rollup/plugin-commonjs/-/plugin-commonjs-21.0.1.tgz",
- "integrity": "sha512-EA+g22lbNJ8p5kuZJUYyhhDK7WgJckW5g4pNN7n4mAFUM96VuwUnNT3xr2Db2iCZPI1pJPbGyfT5mS9T1dHfMg==",
+ "version": "20.0.0",
+ "resolved": "https://registry.npmjs.org/@rollup/plugin-commonjs/-/plugin-commonjs-20.0.0.tgz",
+ "integrity": "sha512-5K0g5W2Ol8hAcTHqcTBHiA7M58tfmYi1o9KxeJuuRNpGaTa5iLjcyemBitCBcKXaHamOBBEH2dGom6v6Unmqjg==",
diff --git a/package.json b/package.json
index 2784a12..ee80f3b 100644
--- a/package.json
+++ b/package.json
@@ -63 +63 @@
- "@rollup/plugin-commonjs": "^21.0.1",
+ "@rollup/plugin-commonjs": "^20.0.0",This took hours to isolate and frankly does not make any sense. The usual suspects - The 5 to 12 not-so-random Upgrading to I personally don't use rollup watch, so it doesn't affect me. I was just putting the finishing touches on a patch to report unfulfilled hook actions to stderr upon abrupt rollup exit and noticed it during testing. |
|
interesting. I don't re-call having seen any test timeouts on my box. 🤔 and if I did, I did a bunch of other things at the same time (building node, building deno etc.) thinking there likely wasn't enough processing power and/or memory available. I'll keep an eye out tho. that said, just noticed, I think we should add a macOS build to the ci matrix.
are they failing all the time, or only sometimes? |
|
100% locally reproducible. I found the problem. I wasn't going crazy after all. It's a genuine bug in --- rollup-dist-built-with-commonjs-20.0.0/shared/rollup.js 2021-12-17 14:23:52.000000000 -0500
+++ rollup-dist-built-with-commonjs-21.0.1/shared/rollup.js 2021-12-17 14:22:47.000000000 -0500
@@ -1,7 +1,7 @@
/*
@license
Rollup.js v2.61.1
- Fri, 17 Dec 2021 19:23:44 GMT - commit 28cc1988bdb39fb47d3a6c24f90cb34e0c9c39ca
+ Fri, 17 Dec 2021 19:22:38 GMT - commit 28cc1988bdb39fb47d3a6c24f90cb34e0c9c39ca
https://github.com/rollup/rollup
@@ -592,36 +592,9 @@
fsEventsImportError = err;
});
}
-// A call to this function will be injected into the chokidar code
-function getFsEvents() {
- if (fsEventsImportError)
- throw fsEventsImportError;
- return fsEvents;
-}
-
-const fseventsImporter = {
- __proto__: null,
- loadFsEvents,
- getFsEvents
-};
var commonjsGlobal = typeof globalThis !== 'undefined' ? globalThis : typeof window !== 'undefined' ? window : typeof global !== 'undefined' ? global : typeof self !== 'undefined' ? self : {};
-function getAugmentedNamespace(n) {
- if (n.__esModule) return n;
- var a = Object.defineProperty({}, '__esModule', {value: true});
- Object.keys(n).forEach(function (k) {
- var d = Object.getOwnPropertyDescriptor(n, k);
- Object.defineProperty(a, k, d.get ? d : {
- enumerable: true,
- get: function () {
- return n[k];
- }
- });
- });
- return a;
-}
-
var charToInteger = {};
var chars$1 = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=';
for (var i = 0; i < chars$1.length; i++) {
@@ -23611,10 +23584,8 @@
exports.defineConfig = defineConfig;
exports.ensureArray = ensureArray$1;
exports.error = error;
-exports.fseventsImporter = fseventsImporter;
exports.generatedCodePresets = generatedCodePresets;
exports.getAliasName = getAliasName;
-exports.getAugmentedNamespace = getAugmentedNamespace;
exports.getOrCreate = getOrCreate;
exports.loadFsEvents = loadFsEvents;
exports.objectifyOptionWithPresets = objectifyOptionWithPresets;Notice that |
|
Based on the finding above I recommend to apply the patch in #4300 (comment) to revert the commonjs plugin rollup is built with. |
|
nice find @kzc ! just got the macos ci to fail, then on v12 the next run: https://github.com/rollup/rollup/runs/4564828157?check_suite_focus=true |
|
do you know if v21.0.0 or v21.0.1 introduced this? edit: it seems 21.0.0 does not fail (which might not mean anything) edit: nm, it's v21.0.0, just did a build diff myself. |
|
Please hold off on #4308. I was on the right track, but the reversion of the commonjs plugin works for the wrong reason. Ironically, rollup built with In the diff below --- rollup-dist-built-with-commonjs-20.0.0/shared/index.js
+++ rollup-dist-built-with-commonjs-21.0.1/shared/index.js
@@ -3056,15 +3056,13 @@
var fseventsHandler = {exports: {}};
-const require$$3 = /*@__PURE__*/rollup.getAugmentedNamespace(rollup.fseventsImporter);
-
const fs$1 = fs$4;
const sysPath$1 = path$1;
const { promisify: promisify$1 } = require$$2;
let fsevents;
try {
- fsevents = require$$3.getFsEvents();
+ fsevents = require('../../../src/watch/fsevents-importer').getFsEvents();
} catch (error) {
if (process.env.CHOKIDAR_PRINT_FSEVENTS_REQUIRE_ERROR) console.error(error);
}Notice that the failing After some experimentation I noticed that the commonjs plugins that pass all watch tests on Mac, namely After the following patch is applied to 28cc198 (which uses --- a/rollup.config.ts
+++ b/rollup.config.ts
@@ -11,3 +11,2 @@ import { terser } from 'rollup-plugin-terser';
import addCliEntry from './build-plugins/add-cli-entry';
-import conditionalFsEventsImport from './build-plugins/conditional-fsevents-import';
import emitModulePackageFile from './build-plugins/emit-module-package-file';
@@ -70,3 +69,2 @@ const nodePlugins = [
json(),
- conditionalFsEventsImport(),
string({ include: '**/*.md' }), |
|
So the outstanding questions are:
Shouldn't that
|
that's weird, I'm guessing this never worked (as intended)? 🤔 unless rollup was published with the transpiled src. edit: nm, you are saying that the v20 cjs build had a different output. |
|
Actually it used to work well, and I understand now why it broke. This require was meant to be internal, and it was actually injected into fsevents by a build plugin. The reason it failed to work is because the require is inside a try-catch, and with the last major, the default auf the ignoreTryCatch option was unfortunately changed to true, ignoring ALL require expressions in try blocks. The commonjs beta plugin fixes this by only ignoring external requires. |
|
The real question is whether |
@kzc just a bad guess: the try/catch require('fsevents') call fails, and chokidar uses the fallback (kqueue) on macos. |
|
I'm not talking about needing Side note: the clear screen for CLI use of so no users could observe it. I recommend to remove the initial clear screen from watch. |
|
The fsevents plugin very much improves watch mode reliability and performance for large projects on Mac. We are lucky nobody complained, but the symptoms would be hard to diagnose correctly anyway. |
|
ok, my bad, should have clarified: 😃
|
If it happened once, it could happen again. This is very difficult to diagnose and debug and wasted hours of time. |
curious: how? is the ignore-try/catch-option going away? |
the only simple way to test this is possibly to check if |
This PR would be sufficient: #4307 |
True, but if we are specifying the option explicitly, even another change in default values should not have an effect. We should definitely add the Mac pipeline, and that one becomes at least flaky if we do not use fsevents (which alone should be a testament to its usefulness), but I think this should be enough of a test. But reading your last comment sounds to me like that is what you meant.
It is not going away, but one of the changes of the beta version is that ignore-try-catch will never ignore internal imports, only external ones. And this is an internal import. |
ah, thanks! I was just wondering, is it not possible to e.g. declare I guess I gotta play with the settings a bit 😄 I usually try to avoid native modules if in any way possible. |
|
The problem is that without the extra logic, the conditional require is converted to an actual import of a native module, which fails miserably. |
that makes sense. what I meant tho, if one currently marks another potential avenue could be to use |
|
it seems the macos |
|
I see 3 watch flakes for node12/mac and 1 flake for node14/linux on master. So it's not specific to Mac. The problem appears to be a race condition in the way the tests are written. Here's an expanded version of the rollup output for the failing node14/linux test case https://github.com/rollup/rollup/runs/4615040404?check_suite_focus=true What I think is going on in these failures is that updated config is being reloaded mid-write and as result rollup is failing to parse the incomplete config file. Let's test the regexp against the test output. If one were to change the watch test regexp in question from: to: that might be an adequate workaround. It would ignore the half-written rollup config file parse error in the middle. |
The cjs plugin could leave a require expression, but that would also remain a require expression for the ESM build as well, or rather a non-existing global variable call. The current solution works correctly for all output formats. |
|
The other flaky watch tests are also due to race conditions in the tests themselves. When github CI is overloaded (which is the norm these days) the watch failures tend to appear more frequently in these flaky tests. If I have time over the holiday I'll see if I can make these tests more robust. The biggest obstacle is not so much the test fixes themselves, but understanding what they are trying to do. |
|
I briefly looked at the watch failures in recent commits for macos and linux and put together this patch which I think could help reduce some flakiness: diff --git a/test/cli/samples/watch/watch-config-early-update/_config.js b/test/cli/samples/watch/watch-config-early-update/_config.js
index c13c269..7a6061e 100644
--- a/test/cli/samples/watch/watch-config-early-update/_config.js
+++ b/test/cli/samples/watch/watch-config-early-update/_config.js
@@ -23,7 +23,7 @@ module.exports = {
format: 'es'
}
}),
- 1000
+ 2000
);
});
`
diff --git a/test/cli/samples/watch/watch-config-no-update/_config.js b/test/cli/samples/watch/watch-config-no-update/_config.js
index 635d77d..a0e31d5 100644
--- a/test/cli/samples/watch/watch-config-no-update/_config.js
+++ b/test/cli/samples/watch/watch-config-no-update/_config.js
@@ -16,14 +16,14 @@ module.exports = {
command: 'rollup -cw',
before() {
configFile = path.resolve(__dirname, 'rollup.config.js');
- fs.writeFileSync(configFile, configContent);
+ atomicWriteFileSync(configFile, configContent);
},
after() {
fs.unlinkSync(configFile);
},
abortOnStderr(data) {
if (data.includes('created _actual/main.js')) {
- fs.writeFileSync(configFile, configContent);
+ atomicWriteFileSync(configFile, configContent);
return new Promise(resolve => setTimeout(() => resolve(true), 500));
}
},
@@ -37,3 +37,12 @@ module.exports = {
}
}
};
+
+// Workaround a race condition in fs.writeFileSync that temporarily creates
+// an empty file for a brief moment which may be read by rollup watch - even
+// if the content being overwritten is identical.
+function atomicWriteFileSync(filePath, contents) {
+ const stagingPath = filePath + "_";
+ fs.writeFileSync(stagingPath, contents);
+ fs.renameSync(stagingPath, filePath);
+}
diff --git a/test/watch/index.js b/test/watch/index.js
index f7d9124..853a7ce 100644
--- a/test/watch/index.js
+++ b/test/watch/index.js
@@ -311,7 +311,7 @@ describe('rollup.watch', () => {
const MAIN_ID = path.resolve('test/_tmp/input/main.js');
let lastEvent = null;
await sander.copydir('test/watch/samples/watch-files').to('test/_tmp/input');
- await wait(100);
+ await wait(200);
watcher = rollup.watch({
input: 'test/_tmp/input/main.js',
output: {I believe the lack of an atomic file write was the cause of the rare failure in Unfortunately for the other two flaky failing tests, all I could offer was to further game the race in favor of passing on overloaded machines, as those tests are quite elaborate and are above my pay grade. github CI machine load variability has been quite high lately. Tests with race conditions will continue to randomly fail from time to time, and the failures will tend to cluster together depending on when the job is run. But it's only significant if a given test consistently fails on a specific platform for several days. |
|
I have created #4318 to investigate these ideas and look into the other watch tests. Discussion should probably continue there. Thanks for the work! |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
just a left-over from: #4293
I thought at the time
core-jswas halting the ci run, so I left it out, but now I'm not really sure if I remember correctly. 😕