-
Notifications
You must be signed in to change notification settings - Fork 518
feat(plugin): wrap enum property values in functions #3629
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
base: master
Are you sure you want to change the base?
Conversation
bde4830 to
3bf3ab4
Compare
- Check identifierText.includes('await ') instead of typeName.includes('import(')
- Update test fixtures to conditionally add async based on esmCompatible
- Add regression test for same-file array types (nestjs#3630)
|
The commit eb5bc19 applied the same async modifier fix pattern to enum properties that was applied to type properties. |
|
@kamilmysliwiec Hello Kamil, I know you’re usually quite busy, but would it be possible for you to review this PR with higher priority? In an ESM environment, using a class or a TypeScript enum as a property type currently results in an incorrect code transformation, causing the application to fail to start at runtime. I’ve also included tests addressing the issue introduced in the 11.2.2 patch (#3630). |
|
Doesn't this change break older projects (same as the previous PR #3630)? |
|
@kamilmysliwiec With the changes introduced in this PR, adding the following code should make it work correctly: if (isPromise(metadata.type)) {
metadata.type = await metadata.type;
}However, since this requires the use of await, a large number of functions would need to become async, and ultimately SwaggerModule.createDocument(app, config) in main.ts would also need to be awaited, which changes the public interface. If the project has decided to rely on await import for ESM compatibility, this change may be unavoidable. What are your thoughts on this? |
In a CommonJS environment, everything works as expected. |
Is there any specific reason why we cant stick to require given that it's now compatible with ESM? |
The CLI plugin’s ESM-compatible mode uses await import instead of require, which leads to the current issue. The change I proposed earlier is intended to address this behavior. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
This class will be converted as:
The enum property has an await statement without an async function wrapping.
Issue Number: N/A
What is the new behavior?
Wrap enum property values in arrow functions to support dynamic imports.
When the enum type reference contains dynamic import syntax, the function is marked as async.
enum: () => EnumTypeenum: async () => EnumTypeimport()syntax and add async modifier accordinglyThis class will be converted as:
Does this PR introduce a breaking change?
Other information
This change may be related to: #3603