[TS] Add generated content to repo to track future changes #7396

bjornharrtell · 2022-07-28T18:22:24Z

allows to run code generation and/or typescript tests without ending up with modified or untracked content
harmonizes code generation check and typescript tests to generate the same content

bjornharrtell · 2022-07-28T18:25:41Z

Strange.. CI complains with:

Run scripts/check_generate_code.py
diff --git a/tests/optional-scalars/scalar-stuff.ts b/tests/optional-scalars/scalar-stuff.ts
index 6b70ded..a7[4](https://github.com/google/flatbuffers/runs/7566214476?check_suite_focus=true#step:4:5)8c22 100[6](https://github.com/google/flatbuffers/runs/7566214476?check_suite_focus=true#step:4:7)44
--- a/tests/optional-scalars/scalar-stuff.ts
+++ b/tests/optional-scalars/scalar-stuff.ts
@@ -20[7](https://github.com/google/flatbuffers/runs/7566214476?check_suite_focus=true#step:4:8),10 +207,6 @@ defaultEnum():OptionalByte {
   return offset ? this.bb!.readInt[8](https://github.com/google/flatbuffers/runs/7566214476?check_suite_focus=true#step:4:9)(this.bb_pos + offset) : OptionalByte.One;
 }
 
-static getFullyQualifiedName():string {
-  return 'optional_scalars.ScalarStuff';
-}
-

bjornharrtell · 2022-07-28T18:43:34Z

Looks like it was due to different gen options for tests vs ./scripts/generate_code.py. Hopefully fixed in additional commit.

bjornharrtell · 2022-07-28T19:01:14Z

This may look like alot of redundant content but I do believe it provides value to be able to see/detect future changes affecting generation. And AFAIK for other langs we already have the generated content in repo.

aardappel

Would like @dbaileychess to review before it goes in

aardappel · 2022-07-28T19:00:32Z

tests/reflection/advanced-features.ts

@@ -0,0 +1,12 @@
+// automatically generated by the FlatBuffers compiler, do not modify


should this and all other related files be in reflection rather than tests ?

do we have TS code that relies on reflection?

this does indeed seem confused between tests/reflection. I don't know anything about it and note that is was also added by #7161.

aardappel · 2022-07-28T19:07:54Z

tests/foobar/abc.js

@@ -0,0 +1,5 @@
+// automatically generated by the FlatBuffers compiler, do not modify


where do these foobar files come from? is this TS specific?

also seem related to #7161 and specifically this included schema - https://github.com/google/flatbuffers/blob/master/tests/test_dir/typescript_transitive_include.fbs

Yeah, these don't have to be saved in the repo. The tests/TypeScripttest.sh should output them to a temp file, assert they exist, and then delete them. If we do that, we don't have to worry about checking them in.

aardappel · 2022-07-28T19:11:22Z

tests/typescript_keywords.ts

@@ -0,0 +1,5 @@
+export { Abc } from './foobar/abc';


these all sit directly in tests which should be avoided.. what are they for anyway? these don't seem to come from the schemas we use with other languages

seems it originates from #7161 with the purpose of testing typescript keyword clashes or something like that

I agree it's messy that generated stuff ends up directly in tests. However, note that is also true for other langs fx. https://github.com/google/flatbuffers/blob/master/tests/monster_test_generated.h.

That is from a language which generates all its code (including namespaces) into a single file. Languages that create a file per class and directories for namespaces should use directories in general.

but the main reason for generated files not ending up in a subdir is that they do not have namespace?

I'll note that the _generated.ts files are generating all their code in a single file (i.e., all the ones generated with --ts-flat-files).

For the "normal" codegen'd files (e.g., the typescript_keywords.ts), I'm not as familiar with the history of why this setup was chosen, but the non-folder'd file is serving as a sort of equivalent to an index.ts where it doesn't contain any actual definitions, and just exports all the relevant types for a given schema file, and so it makes some logical sense to me that it would not be in a random sub-folder.

aardappel · 2022-07-28T20:03:42Z

If all is generated by #7161 and we are not sure what for, we should not be adding these to the repo.

We should only add to the repo files which are directly generated from the schemas we actually have test code written for, and which are shared with other languages.

bjornharrtell · 2022-07-28T20:32:29Z

@aardappel I agree and I cannot find any evidence of them actually being exercised in tests but I assume there is a sensible reason for why they were added, perhaps @jkuszmaul can clarify?

jameskuszmaul-brt · 2022-07-28T23:36:24Z

Those files are "tested" insofar as that the typescript compiler should be building them and type-checking and whatnot without generating errors. I didn't add any run-time logic exercising the modified files since it didn't feel too necessary for the code I was adding (since for the changes I was making, ensuring that the imports and types worked out was really the main concern). That's not to say that more tests wouldn't be good...

It wouldn't really be hard to make it codegen the files in a subdirectory, but as mentioned I think some other languages do the same thing at the top level of the tests/ directory, so I didn't consider it notable at the time.

Edit: Replying from the wrong account, but the same person as @jkuszmaul :)

bjornharrtell · 2022-07-29T09:33:32Z

Thanks @jameskuszmaul-brt, that makes sense to me.

bjornharrtell · 2022-08-04T11:33:11Z

@aardappel are you ok with review comment answers and could we merge?

aardappel · 2022-08-04T16:28:54Z

@bjornharrtell well none of what I asked about has been resolved, right? "that other PR added them and we don't know why" doesn't seem a good reason to add a ton of files in sub-optimal locations.. like I said before, I'd prefer it if we only merged files that are directly under test by our TS testing code, and I'd prefer it if any new files don't unnecessarily land in the root test dir.

Maybe I am wrong about some of this, but find it hard to ok this as-is without someone clearly stating that any of this is needed. I'll ping @dbaileychess

dbaileychess · 2022-08-04T16:31:05Z

@aardappel @bjornharrtell I'll take a look at this today.

dbaileychess · 2022-08-05T16:07:09Z

I agree with @aardappel, we should first figure out/clean up what files are being generated for TS and remove the ones that aren't being directly tested or at least not namespaced.

jkuszmaul · 2022-08-05T16:23:26Z

I feel like there's a disconnect here. As I noted above, the *.ts files get exercised (directly or indirectly via imports) by being compiled by the typescript compiler in tests/TypeScriptTest.sh. This then generates .js files adjacent to the .ts files (which could be changed)--the .js files are not being exercised. Running tsc alone exercises testing keyword clashes in the codegen, as well as any incorrect import paths, and goes a fair ways towards confirming correct codegen.

dbaileychess · 2022-08-05T16:56:00Z

@jkuszmaul Yeah, I understand that part of the test, but the actual output files are of no clear use and shouldn't be checked in. Can we write a test that tsc correctly works and returns OK, and just disregard the output files?

bjornharrtell · 2022-08-05T16:56:55Z

I also feel there is a disconnect. I agree it's unfortunate that some generated end up in the tests root but it's the same for other langs. This only rectifies the incomplete state that when running code generation you otherwise get multiple untracked files in the tree and it's not clear why they are not added as the rest is.

dbaileychess · 2022-08-05T16:59:30Z

Then the test script should delete those files explicitly after verifying tsc worked.

bjornharrtell · 2022-08-05T17:04:36Z

That could make sense. But my argument is that we do not primarily commit generated content for the purpose of runtime tests - for that purpose all it could in fact all be deleted after the test run. We commit it to track effects of code generation logic changes.

dbaileychess · 2022-08-05T17:13:30Z

I think it is a balance, and we do a poor job of it now. We dupe so much stuff in all of our generated code, that at this point just becomes noise. How many times do we need to see the generated output of an integer field for a given language? We probably have 100s of those in all of our generated files.

Ideally we would have a golden/ directory that just had generated output for each language and each feature, neatly namespaced/directorized. Then the test/ directory can just focus on tests that use the generated code. We kind of conflate the two.

bjornharrtell · 2022-08-05T17:37:34Z

I agree it is a bit messy and redundant but it is what it is. It's it not becoming less messy to make an exception for a subset of from TS generated source because it happens to not be exercised by test cases at this time. I would rather argue that it would make sense to add some trivial runtime tests for it bur why make that a requirement at this time?

aardappel · 2022-08-05T18:02:06Z

When I said "used by the tests", I mean there is human written code calling the functions in it. The fact that it gets "compiled" is in theory useful, but it is not necessary.

Other languages are not creating files to this extent (and certainly not in the root dir), so I don't see why we should make things much worse now. I'd prefer this to be cleaned up to the minimum files actually needed.

bjornharrtell · 2022-08-05T18:27:49Z

It's difficult (IMO) to determine "the minimum files actually needed". Also IMO it would make much more sense to say either no generated sources in repo or all of them. I strongly feel that being selective about it is problematic.
As for other langs not pulluting the root I just don't see it.. what about:

arrays_test_generated.h
monster_extra_generated.h
monster_test_bfbs_generated.h
monster_extra_my_game_generated.dart
monster_test_generated.grpc.fb.cc
monster_test_generated.grpc.fb.h
monster_test_generated.h
monster_test_generated.lobster
monster_test_generated.py
it goes on...

bjornharrtell · 2022-08-05T18:46:34Z

And yet another argument is that the generated code in question is of interest to reference and discuss in relation to #7395 which I'm currently still a bit confused about if there is a bug or if it is per design and sensible.

aardappel · 2022-08-05T20:32:49Z

As I already explained before, a file like monster_test_generated.h is not the same thing, as it contains a whole dir worth of type definitions, unlikely TS which generates one file per type, thus should sit in a namespaced dir.

Regardless of what there already is, we should not make things worse, and this PR seems to go much beyond what we have so far in the wrong direction.

I am not sure why it is hard to determine what is needed, this PR is pulling in files which a) are not directly tested by test code and b) there's no equivalent in (most) other languages. It should be possible to figure out how to not generate them and/or not add them to this PR, and/or namespace things, surely.

I get the sense we're arguing what is easiest, not what is good for the project.

dbaileychess · 2022-08-05T23:27:19Z

Let's pause this PR until we clean up what tests/TypeScriptTest.sh outputs. We can do that in a separate PR. I would just have the generated code sent to a temp directory that is deleted at the conclusion.

bjornharrtell · 2022-08-06T09:25:18Z

I'm sorry that we are in such disagreement here.

bjornharrtell · 2022-08-06T18:34:54Z

Hmm, @dbaileychess and @aardappel what about if we create a dedicated folder structure (and/or subfolder in tests, I would suggest one per language) intended for and only for generated code? I could try to make that happen here for TS/JS if that is an acceptable middle ground.

dbaileychess · 2022-08-05T16:09:41Z

tests/TypeScriptTest.sh

-    ../flatc --ts --gen-name-strings --gen-mutable --gen-object-api -I include_test monster_test.fbs
-    ../flatc --gen-object-api -b -I include_test monster_test.fbs unicode_test.json
-    ../flatc --ts --gen-name-strings --gen-mutable --gen-object-api -o union_vector union_vector/union_vector.fbs
+    ../flatc --ts --reflect-names --gen-name-strings --gen-mutable --gen-object-api -I include_test monster_test.fbs


We first should check in the generate code before modifying the arguments, so we can see how it changes.

dbaileychess · 2022-08-05T16:11:15Z

tests/TypeScriptTest.sh

    ../flatc --ts --gen-name-strings optional_scalars.fbs
-    ../flatc --ts --gen-name-strings --gen-object-api --gen-mutable -I ../ ./typescript_keywords.fbs test_dir/typescript_include.fbs test_dir/typescript_transitive_include.fbs ../reflection/reflection.fbs
-    ../flatc --ts --gen-name-strings --gen-object-api --gen-mutable --ts-flat-files -I ../ ./typescript_keywords.fbs test_dir/typescript_include.fbs test_dir/typescript_transitive_include.fbs ../reflection/reflection.fbs
+    ../flatc --ts --reflect-names --gen-name-strings --gen-mutable --gen-object-api -I ../ ./typescript_keywords.fbs test_dir/typescript_include.fbs test_dir/typescript_transitive_include.fbs ../reflection/reflection.fbs


Let's clean this part up to remove generating things that are not directly tested. If they are still valued, we should make sure they are put in an appropriate directory.

dbaileychess · 2022-08-06T20:12:30Z

tests/foobar/abc.js

@@ -0,0 +1,5 @@
+// automatically generated by the FlatBuffers compiler, do not modify


Yeah, these don't have to be saved in the repo. The tests/TypeScripttest.sh should output them to a temp file, assert they exist, and then delete them. If we do that, we don't have to worry about checking them in.

dbaileychess · 2022-08-26T04:00:24Z

I think this will be not needed if we move our lang-specific files to their own directories. At that point, we can check in the files?

bjornharrtell · 2022-08-26T12:30:03Z

I think this will be not needed if we move our lang-specific files to their own directories. At that point, we can check in the files?

Agree. And as I see it doing that is actually resolving this issue.

dbaileychess · 2022-09-13T03:47:08Z

@bjornharrtell Can this be dropped?

bjornharrtell · 2022-09-13T06:09:40Z

@dbaileychess yep, closing.

[TS] Add generated content to repo to track future changes

652dd4a

github-actions bot added javascript typescript labels Jul 28, 2022

[TS] Gen with all opt ins

9a9dfc4

github-actions bot added the python label Jul 28, 2022

aardappel reviewed Jul 28, 2022

View reviewed changes

bjornharrtell mentioned this pull request Jul 29, 2022

[TS] Upgrade TS/node deps and other improvements #7398

Closed

dbaileychess reviewed Aug 6, 2022

View reviewed changes

bjornharrtell mentioned this pull request Sep 4, 2022

[TS/JS] Move TS tests to dedicated folder and deps upgrade #7508

Merged

bjornharrtell closed this Sep 13, 2022

bjornharrtell deleted the ts-add-gen branch September 13, 2022 06:09

		@@ -0,0 +1,12 @@
		// automatically generated by the FlatBuffers compiler, do not modify

		@@ -0,0 +1,5 @@
		// automatically generated by the FlatBuffers compiler, do not modify

[TS] Add generated content to repo to track future changes #7396

[TS] Add generated content to repo to track future changes #7396

Conversation

bjornharrtell commented Jul 28, 2022 • edited

bjornharrtell commented Jul 28, 2022 • edited

bjornharrtell commented Jul 28, 2022

bjornharrtell commented Jul 28, 2022

aardappel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aardappel commented Jul 28, 2022

bjornharrtell commented Jul 28, 2022

jameskuszmaul-brt commented Jul 28, 2022 • edited

bjornharrtell commented Jul 29, 2022

bjornharrtell commented Aug 4, 2022

aardappel commented Aug 4, 2022

dbaileychess commented Aug 4, 2022

dbaileychess commented Aug 5, 2022

jkuszmaul commented Aug 5, 2022 • edited

dbaileychess commented Aug 5, 2022

bjornharrtell commented Aug 5, 2022

dbaileychess commented Aug 5, 2022

bjornharrtell commented Aug 5, 2022 • edited

dbaileychess commented Aug 5, 2022

bjornharrtell commented Aug 5, 2022

aardappel commented Aug 5, 2022

bjornharrtell commented Aug 5, 2022

bjornharrtell commented Aug 5, 2022

aardappel commented Aug 5, 2022 • edited

dbaileychess commented Aug 5, 2022

bjornharrtell commented Aug 6, 2022

bjornharrtell commented Aug 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaileychess commented Aug 26, 2022

bjornharrtell commented Aug 26, 2022

dbaileychess commented Sep 13, 2022

bjornharrtell commented Sep 13, 2022

bjornharrtell commented Jul 28, 2022 •

edited

bjornharrtell commented Jul 28, 2022 •

edited

jameskuszmaul-brt commented Jul 28, 2022 •

edited

jkuszmaul commented Aug 5, 2022 •

edited

bjornharrtell commented Aug 5, 2022 •

edited

aardappel commented Aug 5, 2022 •

edited