Generate flavoured types for conjure aliases #181

matthieu-beteille · 2021-11-17T22:22:22Z

Before this PR

This tackles #23

Conjure aliases would simply be resolved and translated into their referenced equivalent type.

After this PR

conjure-typescirpt will generate flavoured typescript types for "aliases" of the following primitives:

  PrimitiveType.RID,
  PrimitiveType.STRING,
  PrimitiveType.BEARERTOKEN,
  PrimitiveType.BOOLEAN,
  PrimitiveType.DOUBLE,
  PrimitiveType.INTEGER,
  PrimitiveType.SAFELONG,
  PrimitiveType.UUID,
  PrimitiveType.BEARERTOKEN

NOTE: this also bumps TS to 4.4 to be able to support for string and number aliases in map's keys - release note here
This update involved a few changes:

bumping webpack to 5.x
remove use of the deprecated awesome-ts-loader in favour of ts-loader

Tried on a few palantir generated deps, everything seemed fine, but I should try with more dependencies.

Remaining questions:

unsure if we should restrict the scope of this initial PR to only support aliased STRING/RID?
should this be feature flagged so it can be enabled slowly
it feels like better be safe and major break this?
could there be any unwarranted side effects of bumping ts? it seems like the generated package json add a dev dependency on the typescript version used in the project

==COMMIT_MSG==
Convert aliases into flavoured type
==COMMIT_MSG==

Possible downsides?

palantirtech · 2021-11-17T22:22:23Z

Thanks for your interest in palantir/conjure-typescript, @matthieu-beteille! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

changelog-app · 2021-11-17T22:22:26Z

Generate changelog in `changelog/@unreleased`

Type

Description

Convert aliases into flavoured type

Check the box to generate changelog(s)

Generate changelog entry

matthieu-beteille · 2021-11-18T14:39:55Z

src/commands/generate/typeGenerator.ts

+ * Generates a file of the following format:
+ * ```
+ *  export type ExampleAlias = string & {
+ *     __conjure_flavor_type?: "_flavor_ExampleAlias";


TODO: update this

src/commands/generate/typeGenerator.ts

src/commands/generate/utils.ts

src/commands/generate/__tests__/resources/definitions/example-types.yml

src/commands/generate/utils.ts

matthieu-beteille · 2021-12-01T22:22:59Z

...commands/generate/__tests__/resources/test-cases/example-types/product/bearerAliasExample.ts

@@ -0,0 +1 @@
+export type IBearerAliasExample = string & { __conjure_type?: "BearerAliasExample" };


NOTE: should the field value contain the package name as well?

I think it should. This is so aliases with the same name across packages are not compatible. Here the typings would become:

export type IBearerAliasExample = string & { __conjure_type?: "com.palantir.product:BearerAliasExample" };

@ferozco @Que3216 for thoughts

We probably want package name here, otherwise aliases with the same name but from different packages will be assignable to each other (and I don't think we want that).

agree with @patrickszmucer the fully qualified classname sounds like the right thing

It is a common pattern to share identifiers between multiple services e.g. Object RIDs, Object Type Rids. And I'd imagine it'd be pain having to cast these all the time (which version of ObjectRid from the 10 conjure services would you even use to type the ObjectRids in your store?).

I'd guess that 98% of the time developers would want types of the same name from different services to be compatible. There will be some cases where a developer tries to pass an alias of one name into something that accepts a semantically completely different alias but co-incidentally with the same name, but I'd expect they'll be very rare. If you look at the name clashes some will be zero risk e.g. many services probably have a StateRid alias, but it's pretty unlikely you'll get a bug from accidentally passing the one from the wrong service. Some of them may have some risk, e.g. if Gotham has an ObjectId, you could imagine someone accidentally passing that to Foundry. You could argue that in the small number of cases where there could be consequential clashes you should rename your types.

If we went ahead with adding the package, would the proposal here be that services put these rids under a common package name, so you just have 'com.palantir.foundry:ObjectRid' rather than 'com.palantir.foundry.monitoring.ObjectRid', 'com.palantir.foundry.oss.ObjectRid' etc.? I guess the first time we make this change to each service it'd technically be an API break for any Conjure Java clients right?

Note: we'd also have to update the copy conjure excavator to avoid shading certain RIDs.

Four alternative solutions

1. As proposed, add the package straight into the conjure_type

2. Add another __conjure_package property

If we want to provide FE's maximum flexibility would could do:

export type IBearerAliasExample = string & { __conjure_package?: "com.palantir.product" __conjure_type?: "BearerAliasExample" };

And if FE's want they can create package agnostic versions of these types:

export type IObjectRid = { __conjure_type?: "ObjectRid" };

And so use that in their internal store/state:

{ selectedObject: IObjectRid }

Then no cast is needed if you do an OSS search, get back an OSS.ObjectRid, store it in your state, and then send that RID back out to Phonograph. But if you really do want types from different packages to be treated as different then you can.

3. Make it configurable

Allow it to be configured on an alias by alias, or service by service basis.

4. Don't include the package at all

Maybe solution 2 is the nicest?

@raiju for SA (who's been thinking about other Conjure changes to make cross-service stuff easier)

In java code for now RIDs have to be unwrapped/wrapped explicitly when using a Rid from one service into another service? So with the current proposal, I guess we'd end up in the same situation where a cast has to be explicitly made in frontend apps when required (and so we were not - at least yet - proposing the move of RIDs to a common package)

That being said I do like 2 better as well, as it provides more flexibility indeed and allows for keeping a more "neutral" rid in app state if it makes things easier.

Curious @raiju, @ferozco and @patrickszmucer thoughts again on moving forward with approach 2 above

If alias'd, yes, they have to be unwrapped/re-wrapped explicitly. Agreed, I think 2 makes a lot of sense

matthieu-beteille · 2022-01-07T11:17:12Z

Closing this one as this was opened on my fork when I didn't have access to the org - re-opening here: #189

Que3216 · 2022-01-07T12:50:02Z

Guessing you meant: #189

Matthieu Beteille and others added 4 commits November 16, 2021 19:03

wip adding support for flavors

3387664

bump to ts 4.4 and use ts-loader

1546c44

lint

c384f78

add support for other primitives and update test

9d12e8b

update comment

c5fccf8

matthieu-beteille commented Nov 18, 2021

View reviewed changes

src/commands/generate/__tests__/resources/definitions/example-types.yml Show resolved Hide resolved

matthieu-beteille mentioned this pull request Nov 18, 2021

upgrade ts version and webpack version #182

Closed

p-szm reviewed Nov 19, 2021

View reviewed changes

src/commands/generate/utils.ts Show resolved Hide resolved

matthieu-beteille mentioned this pull request Nov 24, 2021

Upgrade ts version and webpack version #184

Merged

matthieu-beteille commented Dec 1, 2021

View reviewed changes

matthieu-beteille closed this Jan 7, 2022

Que3216 mentioned this pull request Jan 7, 2022

Generate flavoured types for conjure aliases #189

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate flavoured types for conjure aliases #181

Generate flavoured types for conjure aliases #181

matthieu-beteille commented Nov 17, 2021 •

edited

Loading

palantirtech commented Nov 17, 2021

changelog-app bot commented Nov 17, 2021

matthieu-beteille Nov 18, 2021

matthieu-beteille Dec 1, 2021

matthieu-beteille Dec 10, 2021 •

edited

Loading

p-szm Dec 10, 2021

ferozco Dec 10, 2021

Que3216 Dec 10, 2021 •

edited

Loading

Que3216 Dec 10, 2021

matthieu-beteille Dec 17, 2021 •

edited

Loading

raiju Dec 17, 2021

matthieu-beteille commented Jan 7, 2022 •

edited

Loading

Que3216 commented Jan 7, 2022

		@@ -0,0 +1 @@
		export type IBearerAliasExample = string & { __conjure_type?: "BearerAliasExample" };

Generate flavoured types for conjure aliases #181

Generate flavoured types for conjure aliases #181

Conversation

matthieu-beteille commented Nov 17, 2021 • edited Loading

Before this PR

After this PR

Remaining questions:

Possible downsides?

palantirtech commented Nov 17, 2021

changelog-app bot commented Nov 17, 2021

Generate changelog in changelog/@unreleased

matthieu-beteille Nov 18, 2021

Choose a reason for hiding this comment

matthieu-beteille Dec 1, 2021

Choose a reason for hiding this comment

matthieu-beteille Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

p-szm Dec 10, 2021

Choose a reason for hiding this comment

ferozco Dec 10, 2021

Choose a reason for hiding this comment

Que3216 Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Four alternative solutions

1. As proposed, add the package straight into the conjure_type

2. Add another __conjure_package property

3. Make it configurable

4. Don't include the package at all

Que3216 Dec 10, 2021

Choose a reason for hiding this comment

matthieu-beteille Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

raiju Dec 17, 2021

Choose a reason for hiding this comment

matthieu-beteille commented Jan 7, 2022 • edited Loading

Que3216 commented Jan 7, 2022

matthieu-beteille commented Nov 17, 2021 •

edited

Loading

Generate changelog in `changelog/@unreleased`

matthieu-beteille Dec 10, 2021 •

edited

Loading

Que3216 Dec 10, 2021 •

edited

Loading

matthieu-beteille Dec 17, 2021 •

edited

Loading

matthieu-beteille commented Jan 7, 2022 •

edited

Loading