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

fix: replace Duplex with Transform #1577

Merged
merged 1 commit into from
Dec 11, 2024
Merged

fix: replace Duplex with Transform #1577

merged 1 commit into from
Dec 11, 2024

Conversation

mshima
Copy link
Member

@mshima mshima commented Dec 11, 2024

Workaround nodejs/node#55270

@@ -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) {

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

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

Choose a reason for hiding this comment

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

Copy link
Member Author

@mshima mshima Dec 11, 2024

Choose a reason for hiding this comment

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

Copy link
Member Author

@mshima mshima Dec 11, 2024

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

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?

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.

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

@mshima mshima marked this pull request as ready for review December 11, 2024 19:15
@mshima mshima merged commit 6b70b9f into yeoman:main Dec 11, 2024
10 checks passed
@mshima mshima deleted the transform branch December 11, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants