Fix export types for typescript client (#6805) #19616

Drago92 · 2024-09-18T15:13:46Z

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh ./bin/configs/*.yaml
./bin/utils/export_docs_generators.sh
```
(For Windows users, please run the script in Git BASH)
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

joscha · 2024-09-30T09:51:25Z

@macjohnny Looking at 598b509 I think this would actually make sense. It would help any post-processing of the TS when bundling, etc. - is there a reason we only do this for Deno?

joscha · 2024-09-30T09:52:21Z

relates to #19484

macjohnny

thanks for your contribution

macjohnny · 2024-09-30T12:03:17Z

docs/generators/fsharp-functions.md

@@ -34,7 +34,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
 |prependFormOrBodyParameters|Add form or body parameters to the beginning of the parameter list.| |false|
 |sortModelPropertiesByRequiredFlag|Sort model properties to place required parameters before optional parameters.| |true|
 |sortParamsByRequiredFlag|Sort method arguments to place required parameters before optional parameters.| |true|
-|sourceFolder|source folder for generated code| |OpenAPI/src|
+|sourceFolder|source folder for generated code| |OpenAPI\src|


please manually revert to /, as this is the standard for linux-based systems

macjohnny · 2024-09-30T12:07:20Z

@macjohnny Looking at 598b509 I think this would actually make sense. It would help any post-processing of the TS when bundling, etc. - is there a reason we only do this for Deno?

IIRC this was because it was introduced with TypeScript 3.8 https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html, so to have a lower version requirement for non-deno code?

joscha · 2024-09-30T12:16:44Z

IIRC this was because it was introduced with TypeScript 3.8 https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html, so to have a lower version requirement for non-deno code?

That would make sense. What is the guidance for our support of TS versions? 3.8 was released on 20 February 2020. 4+ years is a long time ;)

macjohnny · 2024-09-30T13:49:51Z

That would make sense. What is the guidance for our support of TS versions? 3.8 was released on 20 February 2020. 4+ years is a long time ;)

yeah, looking at

openapi-generator/modules/openapi-generator/src/main/resources/typescript/package.mustache

Line 78 in df22ca6

"typescript": "^4.0",

, it seems we now require at least typescript 4.0

joscha · 2024-09-30T14:07:26Z

That would make sense. What is the guidance for our support of TS versions? 3.8 was released on 20 February 2020. 4+ years is a long time ;)

yeah, looking at

openapi-generator/modules/openapi-generator/src/main/resources/typescript/package.mustache

Line 78 in df22ca6

"typescript": "^4.0",

, it seems we now require at least typescript 4.0

Blame says 3.x support was dropped about 2 years ago already in 8c57b66, so I think this pull request is a go?

macjohnny · 2024-09-30T14:25:55Z

Blame says 3.x support was dropped about 2 years ago already in 8c57b66, so I think this pull request is a go?

yes, after the paths are fixed we can merge

joscha · 2024-09-30T14:36:59Z

samples/client/petstore/R-httr2-wrapper/.openapi-generator/FILES

@@ -5,6 +5,7 @@
 .travis.yml
 DESCRIPTION
 NAMESPACE
+README.md


seems like the sort ordering is not kept stable across systems for this one

yes, probably because the samples were generated on a windows machine

Yes, the problem lies here:

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java

Lines 1984 to 1999 in 849b3e8

filesToSort.sort(PathFileComparator.PATH_COMPARATOR);

filesToSort.forEach(f -> {

// some Java implementations don't honor .relativize documentation fully.

// When outDir is /a/b and the input is /a/b/c/d, the result should be c/d.

// Some implementations make the output ./c/d which seems to mix the logic

// as documented for symlinks. So we need to trim any / or ./ from the start,

// as nobody should be generating into system root and our expectation is no ./

String relativePath = removeStart(removeStart(f.toString(), "." + File.separator), File.separator);

if (File.separator.equals("\\")) {

// ensure that windows outputs same FILES format

relativePath = relativePath.replace(File.separator, "/");

}

if (!relativePath.equals(relativeMeta)) {

sb.append(relativePath).append(System.lineSeparator());

}

});

There is some code to make the output platform-independent, however the sorting happens BEFORE, on the original paths with \, which then affects the sorting. We can't just sort after, because sorting a list of relative paths is not straightforward, as the nesting information gets lost.

can you fix it?

trying already. Always tricky to do non-platform specific things with platform-specific objects :)

joscha · 2024-09-30T14:42:27Z

#19725 has the changes and commits from here + latest master + regenerated samples.

joscha · 2024-10-01T07:47:53Z

@Drago92 if you have a second, it would be amazing if you could regenerate in #19726 on your system via:

gh pr checkout 19726

and then

./mvnw clean package 
./bin/generate-samples.sh ./bin/configs/*.yaml
./bin/utils/export_docs_generators.sh

to see if the fix works as expected.

Drago92 · 2024-10-01T11:32:53Z

@Drago92 if you have a second, it would be amazing if you could regenerate in #19726 on your system via:
gh pr checkout 19726
and then
./mvnw clean package 
./bin/generate-samples.sh ./bin/configs/*.yaml
./bin/utils/export_docs_generators.sh
to see if the fix works as expected.

Your fix works and the slashes are correct now. But the line Endings will be converted on Windows:

and in .dist-Files a "\" will be added after src:

joscha · 2024-10-01T14:16:33Z

Thank you for checking @Drago92 - I didn't fix the first or last image, that is a separate codepath. I didn't touch the line-endings, they are \n currently, probably if you saved the file they will become \r\n. We could possibly add a .gitattributes file with:

FILES text eol=lf

which should then keep them as \n. You could try that if you wanted?

Drago92 · 2024-10-02T11:54:17Z

Thank you for checking @Drago92 - I didn't fix the first or last image, that is a separate codepath. I didn't touch the line-endings, they are \n currently, probably if you saved the file they will become \r\n. We could possibly add a .gitattributes file with:
FILES text eol=lf
which should then keep them as \n. You could try that if you wanted?

Yeah it will help :)

Kevin Broja added 2 commits September 18, 2024 16:32

Fix export type for typescript interfaces in all platforms (OpenAPITo…

598b509

…ols#6805)

Generated samples and docs

d6fc9df

macjohnny requested changes Sep 30, 2024

View reviewed changes

joscha reviewed Sep 30, 2024

View reviewed changes

joscha mentioned this pull request Sep 30, 2024

fix(typescript): export types #19725

Merged

macjohnny closed this in #19725 Sep 30, 2024

joscha mentioned this pull request Sep 30, 2024

fix(core): system-independent relative path sorting for FILES output #19726

Merged

5 tasks

Drago92 deleted the Fix-export-types-for-TypescriptClient-(#6805) branch October 1, 2024 07:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix export types for typescript client (#6805) #19616

Fix export types for typescript client (#6805) #19616

Drago92 commented Sep 18, 2024 •

edited

Loading

joscha commented Sep 30, 2024

joscha commented Sep 30, 2024

macjohnny left a comment

macjohnny Sep 30, 2024

macjohnny commented Sep 30, 2024

joscha commented Sep 30, 2024

macjohnny commented Sep 30, 2024

joscha commented Sep 30, 2024

macjohnny commented Sep 30, 2024

joscha Sep 30, 2024

macjohnny Sep 30, 2024

joscha Sep 30, 2024

macjohnny Sep 30, 2024

joscha Sep 30, 2024 •

edited

Loading

joscha Sep 30, 2024

joscha commented Sep 30, 2024

joscha commented Oct 1, 2024 •

edited

Loading

Drago92 commented Oct 1, 2024 •

edited

Loading

joscha commented Oct 1, 2024

Drago92 commented Oct 2, 2024

	filesToSort.sort(PathFileComparator.PATH_COMPARATOR);
	filesToSort.forEach(f -> {
	// some Java implementations don't honor .relativize documentation fully.
	// When outDir is /a/b and the input is /a/b/c/d, the result should be c/d.
	// Some implementations make the output ./c/d which seems to mix the logic
	// as documented for symlinks. So we need to trim any / or ./ from the start,
	// as nobody should be generating into system root and our expectation is no ./
	String relativePath = removeStart(removeStart(f.toString(), "." + File.separator), File.separator);
	if (File.separator.equals("\\")) {
	// ensure that windows outputs same FILES format
	relativePath = relativePath.replace(File.separator, "/");
	}
	if (!relativePath.equals(relativeMeta)) {
	sb.append(relativePath).append(System.lineSeparator());
	}
	});

Fix export types for typescript client (#6805) #19616

Fix export types for typescript client (#6805) #19616

Conversation

Drago92 commented Sep 18, 2024 • edited Loading

PR checklist

joscha commented Sep 30, 2024

joscha commented Sep 30, 2024

macjohnny left a comment

Choose a reason for hiding this comment

macjohnny Sep 30, 2024

Choose a reason for hiding this comment

macjohnny commented Sep 30, 2024

joscha commented Sep 30, 2024

macjohnny commented Sep 30, 2024

joscha commented Sep 30, 2024

macjohnny commented Sep 30, 2024

joscha Sep 30, 2024

Choose a reason for hiding this comment

macjohnny Sep 30, 2024

Choose a reason for hiding this comment

joscha Sep 30, 2024

Choose a reason for hiding this comment

macjohnny Sep 30, 2024

Choose a reason for hiding this comment

joscha Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

joscha Sep 30, 2024

Choose a reason for hiding this comment

joscha commented Sep 30, 2024

joscha commented Oct 1, 2024 • edited Loading

Drago92 commented Oct 1, 2024 • edited Loading

joscha commented Oct 1, 2024

Drago92 commented Oct 2, 2024

Drago92 commented Sep 18, 2024 •

edited

Loading

joscha Sep 30, 2024 •

edited

Loading

joscha commented Oct 1, 2024 •

edited

Loading

Drago92 commented Oct 1, 2024 •

edited

Loading