fix: replace Duplex with Transform#1577
Conversation
| { filter, ...memFsPipelineOptions }, | ||
| ...transforms, | ||
| Duplex.from(async function* (generator: AsyncGenerator<MemFsEditorFile>) { | ||
| for await (const file of generator) { |
There was a problem hiding this comment.
Instead of using Transform, could you try to add a try catch block around the for loop?
Same for other places you linked the PR
There was a problem hiding this comment.
I will need to hit the hay now but if it works then I can explain when I wake up, cheers!
There was a problem hiding this comment.
The issue persists with the change.
The issue origin is in prettier transform:
https://github.com/jhipster/generator-jhipster/blob/9febd8f5d1a62855de8a4f5d2fafe0869186289b/generators/bootstrap/support/prettier-support.ts#L79-L84
There was a problem hiding this comment.
We have 10 or more transforms:
https://github.com/jhipster/generator-jhipster/blob/9febd8f5d1a62855de8a4f5d2fafe0869186289b/generators/bootstrap/generator.ts#L230-L262
Others in api execution .
There was a problem hiding this comment.
I've tried to identify transforms that are not in objectMode.
Every transform seems to be in objectMode.
Duplex.from with generator is in readableObjectMode and writableObjectMode, but seems to be the source of the issue.
There was a problem hiding this comment.
if you add NODE_DEBUG=stream npx jhipster --defaults (using node 22.5.x) you would see the error was actually being caught but not being properly handled by the cleanup function.
logs
STREAM 46245: onerror Error: Error parsing file jest.conf.js: SyntaxError: Missing initializer in destructuring declaration. (1:34)
> 1 | const { pathsToModuleNameMapper } require('ts-jest');
| ^
2 |
3 | const {
4 | compilerOptions: { paths = {}, baseUrl = './' },
At: 1: const { pathsToModuleNameMapper } require('ts-jest');
2:
3: const {
4: compilerOptions: { paths = {}, baseUrl = './' },
5: } = require('./tsconfig.json');
6: const environment = require('./webpack/environment');
7:
8: module.exports = {
9: transformIgnorePatterns: ['node_modules/(?!.*\\.mjs$|dayjs/esm)'],
10: resolver: 'jest-preset-angular/build/resolvers/ng-jest-resolver.js',
11: globals: {
12: ...environment,
13: },
14: roots: ['<rootDir>', `<rootDir>/${baseUrl}`],
15: modulePaths: [`<rootDir>/${baseUrl}`],
16: setupFiles: ['jest-date-mock'],
17: cacheDirectory: '<rootDir>/target/jest-cache',
18: coverageDirectory: '<rootDir>/target/test-results/',
19: moduleNameMapper: pathsToModuleNameMapper(paths, { prefix: `<rootDir>/${baseUrl}/` }),
20: reporters: [
21: 'default',
22: ['jest-junit', { outputDirectory: '<rootDir>/target/test-results/', outputName: 'TESTS-results-jest.xml' }],
23: ['jest-sonar', { outputDirectory: './target/test-results/jest', outputName: 'TESTS-results-sonar.xml' }],
24: ],
25: testMatch: ['<rootDir>/src/main/webapp/app/**/@(*.)@(spec.ts)'],
26: testEnvironmentOptions: {
27: url: 'https://jhipster.tech',
28: },
29: };
30:
at Object.<anonymous> (file:///Users/H449664/projects/oss/fs-issues/jsp/node_modules/generator-jhipster/dist/generators/bootstrap/support/prettier-support.js:69:23)
at async Object.<anonymous> (file:///Users/H449664/projects/oss/fs-issues/jsp/node_modules/p-transform/dist/index.js:10:5)
at async file:///Users/H449664/projects/oss/fs-issues/jsp/node_modules/p-transform/dist/queue.js:68:28
at async file:///Users/H449664/projects/oss/fs-issues/jsp/node_modules/p-queue/dist/index.js:187:36
STREAM 46245: unpipe
STREAM 46245: call pause
STREAM 46245: pause
STREAM 46245: onunpipe
STREAM 46245: cleanup
This could possibly relates to how nodejs internal handles async generator, since you have found a workaround for your need, I will try to see if I can workout a minimum repro myself and I think I might've introduced this bug in 22.5 since I made a change in nodejs/node@8e5d88b
Thanks for taking time to respond, cheers!
There was a problem hiding this comment.
Thanks @jakecastelli.
Since this affects current and past releases, would be better to have an issue associated with.
Should I open a new issue?
There was a problem hiding this comment.
actually that commit I linked might not be the issue - I tried to build locally without that commit and the issue still persists.
There was a problem hiding this comment.
would be good to have an issue but usually we need a minimum repro first otherwise it becomes less meaningful
Workaround nodejs/node#55270