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

feat: Generate Index Files #821

Merged
merged 7 commits into from
May 23, 2023
Merged

Conversation

blorgon1
Copy link
Contributor

@blorgon1 blorgon1 commented May 3, 2023

[#212] These changes add an outputIndex option for generating index files, based on the proto package namespaces.

An index file is generated for each sub-namespace of a package, so that they can be imported using the same path as the proto package, without relying on TS namespaces.

Example - Given the following proto files:

/// a.proto
syntax = "proto3";
package com.test;

message A {
    string a = 1;
}

/// b.proto
syntax = "proto3";
package com.test;

message B {
    string b = 1;
}

Three index files will be generated:

/// index.ts
export * as com from "./index.com";

/// index.com.ts
export * as test from "./index.com.test";

/// index.com.test.ts
export * from "./a";
export * from "./b";

The index can then be used as:

import protos from './protos'; // the pb output directory
type A = protos.com.test.A;

@blorgon1 blorgon1 changed the title [#212] Generate Index Files feat: Generate Index Files May 3, 2023
@blorgon1
Copy link
Contributor Author

blorgon1 commented May 3, 2023

It might be better to change the final type imports from:

export * as a from "./a";

to:

export * from "./a";

I think that since all of the exports are from the same package, there will be no name collisions because protoc doesn't allow duplicate names in the same package.

The only change to the exports that will be required is to omit exporting DeepPartial and Exact, which can be exported in the root index file instead.

Edit: applied this change

@stephenh
Copy link
Owner

stephenh commented May 3, 2023

Hi @blorgon1 , thanks for the PR!

Can you pick either an existing integration/ test, or create a new one, that uses the new flag, and check in the generated output as part of this PR? This will basically act as a regression test to make sure we don't break the option in the future.

@blorgon1
Copy link
Contributor Author

blorgon1 commented May 3, 2023

@stephenh no problem!

I've added a new integration test, although it's a bit lacking because the integration codegen only processes one proto .bin at a time so it's not possible to test the result of generating indexes for mutliple proto files. Probably good enough to act as a regression test though.

@stephenh
Copy link
Owner

stephenh commented May 6, 2023

Hi @blorgon1 ! I had a chance to review the PR. The nested exports initially threw me off, but reading your PR description, I get your goal, i.e. the package com.test.domain w/AuthorMessage in it turns into:

const AuthorMessage = protos.com.test.domain.AuthorMessage;

It is nice that this approach avoids conflicts, b/c each package gets its own output.

Fwiw I have two musings:

  1. I can anticipate some users wanting a "flat" / package-less export, i.e. even at the risk of conflicts, if they don't have conflicts, wanting to do something like:
imports { AuthorMessage } from "src/ts-proto-output";

No need to build that in your PR, just thinking that maybe someday we'd have an outputIndex=flat or outputIndex=packageless option someday.

  1. I'm wondering how strongly you feel about the dotted package syntax, b.c. if we snake or camel cased the proto package name, we could probably have just 1 index file, i.e.:
  • Given a.proto w/package com
  • And b.proto w/package com.test

We could generate a single index.ts that looked like:

export * as com from "./a";
export * as comTest from "./b";

Then use it as:

import { comTest } from "./src/ts-proto-outout";
const a = comTest.Author...

Granted, you've already got the export-N-levels of working, but, dunno, I think the camel-case-the-name seems simpler to implement and possibly consume, just b/c there are less "shell packages" that exist solely to mimic namespaces.

I'm not completely against the current approach, but do have a general preference for using something like camelized package names to avoid the N-level exports; what do you think?

@blorgon1
Copy link
Contributor Author

blorgon1 commented May 8, 2023

Hi @stephenh, thanks for the review.

  1. Agreed I could see flat exports being useful in some cases, something to consider later.
  2. I prefer the nested package exports because it feels cleanest to use - they're readable and intellisense plays nice with it. I think that camelized package names would produce long and unreadable names as projects grow, and the root index would be very large making it harder to locate exports. Name collisions are another consideration albeit pretty minor.
    • Fwiw in smaller projects it probably doesn't make any difference, because if the package name is omitted then everything is exported from the main index anyways.
    • I would have preferred to use TS namespaces to handle the nesting but as some comments in the original issue mentioned TS namespaces are lacking in features.

@blorgon1
Copy link
Contributor Author

Hi @stephenh apologies for the bump on this, are there any further changes you think should be made here? I was hoping to start using it in one of my projects soon (without having to use submodules) 😄

@blorgon1
Copy link
Contributor Author

@stephenh tests and lint should pass now, can this PR be merged?

@stephenh
Copy link
Owner

Hey @blorgon1 , thanks for the pings! Yep, the tests pass, and I'll go ahead and merge.

Just fwiw I am still a little hesitant about the "imports to mimic namespaces" approach, so on the very low chance that I end up having to debug/support this in more esoteric cases, I can't promise to not fallback to a less-Russian-doll-of-imports approach, but hopefully we won't need to do that.

Thanks for the PR! This has been a popular ask for a long time!

@stephenh stephenh merged commit 85bf206 into stephenh:main May 23, 2023
stephenh pushed a commit that referenced this pull request May 23, 2023
# [1.148.0](v1.147.3...v1.148.0) (2023-05-23)

### Features

* Generate Index Files ([#821](#821)) ([85bf206](85bf206))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.148.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants