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.

Sign up for GitHub

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

fix: replace Duplex with Transform #1577

Merged
merged 1 commit into from
Dec 11, 2024
Merged

fix: replace Duplex with Transform #1577

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/actions/lifecycle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { dirname, isAbsolute, resolve as pathResolve, relative } from 'node:path';
import { pathToFileURL } from 'node:url';
import { createRequire } from 'node:module';
import { Duplex } from 'node:stream';
import { Transform } from 'node:stream';
import { stat } from 'node:fs/promises';
import createDebug from 'debug';
import type { BaseGenerator, ComposeOptions as EnvironmentComposeOptions, GetGeneratorOptions } from '@yeoman/types';
Expand Down Expand Up @@ -679,11 +679,12 @@ export abstract class TasksMixin {
env.sharedFs.pipeline(
{ filter, ...memFsPipelineOptions },
...transforms,
Duplex.from(async function* (generator: AsyncGenerator<MemFsEditorFile>) {
for await (const file of generator) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will need to hit the hay now but if it works then I can explain when I wake up, cheers!

Copy link
Member Author

@mshima mshima Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@mshima mshima Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 10 or more transforms:
https://github.com/jhipster/generator-jhipster/blob/9febd8f5d1a62855de8a4f5d2fafe0869186289b/generators/bootstrap/generator.ts#L230-L262

Others in api execution .

Copy link
Member Author

@mshima mshima Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@jakecastelli jakecastelli Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jakecastelli.
Since this affects current and past releases, would be better to have an issue associated with.
Should I open a new issue?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually that commit I linked might not be the issue - I tried to build locally without that commit and the issue still persists.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to have an issue but usually we need a minimum repro first otherwise it becomes less meaningful

new Transform({
objectMode: true,
transform(file: MemFsEditorFile, _encoding, callback) {
step('Completed', relative(env.logCwd, file.path));
yield file;
}
callback(null, file);
},
}),
),
{ disabled, name },
Expand Down
Loading