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

Referenced assemblies transpilation #39

Merged
merged 7 commits into from
Sep 17, 2022

Conversation

nenoNaninu
Copy link
Owner

Related #37 #38

@nenoNaninu
Copy link
Owner Author

Lack of test cases for new features.

@nenoNaninu
Copy link
Owner Author

nenoNaninu commented Sep 10, 2022

  • add tests
  • update README.md

@Vulthil
Copy link
Contributor

Vulthil commented Sep 10, 2022

This will probably do the trick, however, it will also transpile classes which reference the attribute from different assemblies, which you are NOT referencing directly in the targeted .csproj file.

Meaning that if you have a reference to XYZ.dll, but you aren't using any types from that assembly, they will be included in the transpiled Typescript.

@nenoNaninu
Copy link
Owner Author

I'm filtering on the condition of whether the attribute is annotated here. Still, does such a problem occur?

@Vulthil
Copy link
Contributor

Vulthil commented Sep 10, 2022

I'm filtering on the condition of whether the attribute is annotated here. Still, does such a problem occur?

Yes, imagine someone has a reference to Tapper.Attributes and include the attribute, but it's totally unrelated to the transpiling you are performing.

@nenoNaninu
Copy link
Owner Author

nenoNaninu commented Sep 10, 2022

OK, I understand what you are saying. That behavior is appropriate.

If TapperAttributes is used in another assembly (dll), that assembly is library code. Definitely.

If TapperAttributes is used in a library, it is designed to interact with the front end. Therefore, it is appropriate to target it for transpile.

@Vulthil
Copy link
Contributor

Vulthil commented Sep 10, 2022

What if it is used in an Assembly/Library which is not under the control of the programmer, but still references the Attribute because the referenced Library also has Tapper.Attributes ?

Why would those class need to be transpiled, unless explicitely referenced by the classes from the targeted .csproj?

@nenoNaninu
Copy link
Owner Author

What if it is used in an Assembly/Library which is not under the control of the programmer, but still references the Attribute because the referenced Library also has Tapper.Attributes ?

If the library references Tapper.Attributes and has [TranspilationSource] annotated to the type, it will be transpile to TypeScript if you passed "-asm true" as a command line argument.

@Vulthil
Copy link
Contributor

Vulthil commented Sep 10, 2022

Yes, but what if it's a Library from "XYZ Company" which is not relevant to transpile, but they also use Tapper Internally ?

@nenoNaninu
Copy link
Owner Author

Does the "XYZ Company" library communicate with the front end? If it communicates with the front end, then it is correct to transpile it. If the library does not provide the ability to communicate with the front end, but is using Tapper, then the "XYZ Company" library has the incorrect architecture. Tapper does not care about that problem(incorrect architecture).

Well, in most cases, tapper users will not encounter such problems if they use the default value (-asm false), since only code under their control will be transpiled.

@Vulthil
Copy link
Contributor

Vulthil commented Sep 10, 2022

What do you mean "communicate with the frontend" ?

Tapper is only for generate Typescript types. No communication.

@Vulthil
Copy link
Contributor

Vulthil commented Sep 10, 2022

If you merge this, you will potentially introduce unwanted Typescript types in the transpiled output.
Unwanted types that you do not intend to use, and are not referenced by any type in your compilation.

@nenoNaninu
Copy link
Owner Author

nenoNaninu commented Sep 10, 2022

TypeScript types are generated in order to allow C# (backend) and TypeScript (frontend) applications to communicate with each other. So Tapper generates TypeScript code suitable for serializers such as JSON/msgpack. I do not consider generating TypeScript from C# for any other use case.

@Vulthil
Copy link
Contributor

Vulthil commented Sep 10, 2022

All I'm saying is that unwanted types might be generated.
That is all.

If you look at https://github.com/Vulthil/Tapper/blob/67f506b6253da6425a82c2914ce234386ebc321b/src/Tapper/ReferencedTypeCollector.cs#L23, the function is only looking at Properties and fields and base types of types already being considered for transpialtion, and not types that just happen to have the attribute in some random assembly.

@nenoNaninu
Copy link
Owner Author

If your use case is one that I think is valid, I will consider it. Otherwise, this PR will be merged as is. Also, I want the feature of this PR for myself.

@Vulthil
Copy link
Contributor

Vulthil commented Sep 10, 2022

You are free to do whatever you see fit. It is your project after all.

But consider this scenario:

Someone has a backend project, using Tapper Attributes in order to provide a TypedSignalRClient for both Typescript and C#.

I make a C# project, which uses their Typed C# client in order to communicate with them.
I also provide a SignalR hub, which does NOT depend on any types from the other project mentioned earlier.
I now want to generate Typescript types for my Typed Typescript Client using Tapper.

I will end up with the Types from the first Backend project being transpiled into my typescript types, which was not my intention.

@nenoNaninu
Copy link
Owner Author

Are you saying that end users will only have access to the Hub you provided?

I also provide a SignalR hub, which does NOT depend on any types from the other project mentioned earlier.

If so, I think that -asm false would be sufficient if it is not dependent on an external project.

@Vulthil
Copy link
Contributor

Vulthil commented Sep 10, 2022

A project structure like:

SomeProject.SignalR

  • SomeProject.SignalR.Library

would not work with -asm false in the scenario provided before.

And with -asm true, the types from an arbirary 3rd party might be transpiled to typescript aswell.

@nenoNaninu
Copy link
Owner Author

I am beginning to think that adding the ability to transpile only specific namespaces is a good idea.

@nenoNaninu nenoNaninu merged commit 1900c58 into main Sep 17, 2022
@nenoNaninu nenoNaninu deleted the referenced_assemblies_transpilation branch September 17, 2022 06:56
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