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

Export types with export type #1284

Merged
merged 8 commits into from
Jun 5, 2024
Merged

Export types with export type #1284

merged 8 commits into from
Jun 5, 2024

Conversation

libre-man
Copy link
Contributor

Description of changes: Some types were exported with export instead of export type which is causing issues in our build system. This indicates more clearly that a type is exported, and no runtime behaviour is changed.

@libre-man libre-man requested review from a team as code owners May 21, 2024 10:25
@kuhe
Copy link
Contributor

kuhe commented May 22, 2024

This is a problem with the build system, since type export/import statements are optional.

What is the error?

@libre-man
Copy link
Contributor Author

The problem is that we strongly prefer to consume the source code (better go-to-definition, faster builds for us), not the compiled code. Webpack then complains about it not being to find the things being exported here, as they refer to types.

What would be the downside of having these exported as types instead of normal exports?

@kuhe
Copy link
Contributor

kuhe commented May 23, 2024

You generate the client source code and merge it into a larger compiled application? That's fine.

I'll let the team know to review this PR and we will likely merge it, but the real fix is going to be in your webpack configuration.

You can also configure a post-generation step. You may have a linting/formatting transform applied after code generation, and in that you can add https://typescript-eslint.io/rules/consistent-type-exports/ or https://biomejs.dev/linter/rules/use-export-type/.

@kuhe kuhe self-assigned this May 23, 2024
Comment on lines +387 to +388
writer.write("export type { __MetadataBearer };");
writer.write("export { $$Command };");
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines 117 to 118
writer.write("export type { RuntimeExtension } from \"./runtimeExtensions\";");
writer.write("export type { $LExtensionConfiguration } from \"./extensionConfiguration\";", normalizedClientName);
Copy link
Contributor

Choose a reason for hiding this comment

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

@syall
Copy link
Contributor

syall commented May 23, 2024

@libre-man It looks like the Java checkstyle is failing, can you resolve the errors?

@kuhe
Copy link
Contributor

kuhe commented May 23, 2024

IndexGenerator.java:118: Line is longer than 120 characters (found 122). [LineLength]

@libre-man
Copy link
Contributor Author

@libre-man It looks like the Java checkstyle is failing, can you resolve the errors?

Done @syall!

@libre-man
Copy link
Contributor Author

libre-man commented May 30, 2024

Anything I need to do get this merged still?

@kuhe

@kuhe kuhe merged commit 3798bc1 into smithy-lang:main Jun 5, 2024
10 checks passed
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.

3 participants