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

Generate files into different output units #237

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

danielkaneider
Copy link
Contributor

The idea was to split the output into separate files, instead of putting everything into one output. First, in bigger projects a lot of files might get generated. Second, the mapPackagesToNamespaces wasn't really handy when it comes to using it actually in typescript.

Comments to the implementation:

  • the goal was to remain API compatible with previous version. Therefore, currently the outputFileType has been extended. A slightly cleaner (but breaking) solution would have been, to change the mapPackagesToNamespaces to mapPackages = {toNamespaces, toFiles} for example
  • I also allowed extensions to append to those files, by adding a dedicated interface for EmitterExtensions

@vojtechhabarta
Copy link
Owner

@danielkaneider thanks for your interest in typescript-generator and this PR but this is functionality I don't want to add to typescript-generator.

Splitting output to multiple files increases complexity of already complex problem. Currently typescript-generator supports following module/namespace variants: http://www.habarta.cz/typescript-generator/doc/ModulesAndNamespaces.html. All these variants should be supported when splitting output so it would add another dimension to this problem (I can imagine splitting by package, by declaration and maybe some others).

While I see you did very good job with this PR I still think it is not worth the increased complexity. Output splitting not only increases complexity of typescript-generator code but also increases complexity for typescript-generator users (because they would have more variants to explore, more configuration values). As a Java developer I know people are used to have each class in separate file but in TypeScript/JavaScript it is not often the case. In my opinion here splitting large output file into bunch of smaller files doesn't provide much benefits, there are still many declarations just organized differently.

I think one output file has some advantages. It is easier to track in source control (if it is tracked), it is simpler to use (import), simpler to put into some documentation or just send in email. TypeScript compiler itself is good example of not splitting code into many source files.

I know many people will disagree. For me it is also important to organize my code as I wish but in this case it is just generated code. I think in general it is good to have all possibilities hot to write code - bunch of smaller TypeScript files or one large etc. but again I think this is different case so it should not matter so much.

To all readers: feel free to use 👍 or 👎 to provide feedback.

@danielkaneider
Copy link
Contributor Author

@vojtechhabarta I know that the generator supports modules and namespaces. I see the different output units as a replacement for namespaces for example, not as an addition. Ambient modules might be different, but it shouldn't be hard to add support for them, or at least add some validation that the combination is not supported (yet).

Let me explain the reason for this PR. I like the generated output very much, since it is very neat. But it is a little bit cumbersome to use, from a Java and TS perspective. We've been using namespaces in order to avoid name collisions. We used the generated type(s) like:
import * from 'types.ts'
The import wasn't an issue, but a usage of some class like:
let someVariable: a.b.c.d.e.f.g.h.SomeType;
This full import needs to be used in every place. I know that TS supports import aliases for namespaces, but you always need at least one prefix like alias.SomeType.

The more elegant usage of types would just be
let someVariable: SomeType;
Having an import with
a/b/c/d/e/f/g/h/sometype.ts
is not a problem, since this is common in TS, and IDEs are helping out quiet good here.

I also thought about adding all the logic into one extension, but that would have gotten even worse. Please let me know if you have any idea how we could make that thing less complex

@vojtechhabarta
Copy link
Owner

I see two possibilities how to deal with large set of classes:

Resolving name conflicts using customTypeNaming or customTypeNamingFunction parameter

Parameter customTypeNaming allows to specify different name for classes one by one.
Parameter customTypeNamingFunction can be used for more advanced logic.
For example it is possible to prefix some logical API.

function(name, simpleName) {
    if (name.startsWith('a.b.c.d.old.')) return 'V1' + simpleName;
    if (name.startsWith('a.b.c.d.new.')) return 'V1' + simpleName;
    if (name.startsWith('e.f.g.h.old.')) return 'V2' + simpleName;
    if (name.startsWith('e.f.g.h.new.')) return 'V2' + simpleName;
}

In this case I think namespaces are not needed.

Here is description of the parameters: http://www.habarta.cz/typescript-generator/maven/typescript-generator-maven-plugin/generate-mojo.html

Aliasing imported symbols

Bellow is an example how to use aliases when importing namespaces.

Let's say we have following types.ts module file:

export namespace a.b.c.d.e.f.g.h {
    export interface SomeInterface {
        aaa?: string;
    }
    export class SomeClass {
        bbb?: string;
    }
}

then a.b.c.d.e.f.g.h namespace can be aliased and used like this:

import { a } from "./types";
import h = a.b.c.d.e.f.g.h;
let x: h.SomeInterface = {};
let y: h.SomeClass = new h.SomeClass();

but it is also possible to alias directly SomeInterface and SomeClass members of the h namespace:

import { a } from "./types";
import SomeInterface = a.b.c.d.e.f.g.h.SomeInterface;
import SomeClass = a.b.c.d.e.f.g.h.SomeClass;
let x: SomeInterface = {};
let y: SomeClass = new SomeClass();

Here is description: https://www.typescriptlang.org/docs/handbook/namespaces.html#aliases

Unfortunately these namespace import aliases don't have special support in IDEs as module imports now have. And we can't even expect this support in the future because nowadays modules are mainstream way how to structure the code.

@danielkaneider
Copy link
Contributor Author

I'm aware of those options. Both are not really usable in an enterprise application: the first one needs the be adapted every time the generator fails. The second one is, as you pointed out, cumbersome to use due to bad IDE support. And then I'm wondering why namespaces (which are not IDE friendly) are supported within the generator, whilst file name imports are not. The latter one is quite common, too, and has also good support in IDEs.

@jtoplak
Copy link
Contributor

jtoplak commented Sep 22, 2018

Nice PR. Having this functionality as a extension makes it much more accommodating to those who do not wish to adopt the added complexity. That being said, adding tests to support the added complexity of this extension would be beneficial.

@danielkaneider
Copy link
Contributor Author

@jtoplak I also thought about an extension first. For that to work, the Emitter framework must be more flexible and interchangeable. I'm not sure if such a PR would be more accepted.

@vojtechhabarta
Copy link
Owner

Making Emitter part more extensible would be good. I don't know what exactly would be needed to allow this extension to work but maybe it could be done without too much changes just for this particular extension.

@danielkaneider
Copy link
Contributor Author

Recently I cleaned the PR a little bit up, in order to rebased more easily for future versions. A few remarks to the current Output and Emitter:

  • the current one-output-for-all approach works for TS, but even for some build-in functionality like generateInfoJson and generateNpmPackageJson there is a need to generate some more output files
  • generateInfoJson and generateNpmPackageJson could likely be implemented using regular extensions, if Output would support outputs to different files
  • those 2 methods are using several Settings which are not directly to the TS generation, but rather on how the output of those methods looks like (eg. npmName, npmVersion)
  • it would be nice for extensions to add some code after (or even before) the output. I think there have been some requests for such use-cases before. It would allow outputting constants for each class, for example.
  • Settings contains items, which are crucial to outputs like newLine and quotes; not a problem, just an observations, as Output is internally hiding this.
  • Emitter contains a lot of private logic to generate the TS file. It would (already) be possible to create an extension which outputs different files, but it would have 2 drawbacks: 1) a lot of code would have to be copied/maintained separately, 2) there would be some performance impact, as all the models would be generated twice.

Some suggestions

  • Output should be able to generate additional output-instances based to relative paths
  • extensions should get a more powerful Output (which allows generating additional files), instead of just getting a Writer
  • Ideally DirectoryEmitter could be supported built-in, as it just contains logic for 1) redirecting the output and 2) generating file imports. Other logic is being re-used from the Emitter (where I needed to make some methods protected).
  • if not, then it would be nice to make some logic more re-usable. emitReferences, emitImports could be kind-of static. It would be nice to be able to call methods like emitElements and emitBean somehow.
  • Settings could include a property to specify a custom Emitter, or to not emit at all (in the case we can upgrade the extension functionality)
  • outputs for infoJson and npmPackage could be implemented as extensions
  • nice to have, it there would be a way to pass custom settings as key-value pairs to extensions.

And I'm aware that some of the things might break backward-compatibility.

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.

None yet

4 participants