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.

Sign up for GitHub

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(jsii): added a symbol identifier to every type in the assembly #3030

Merged
merged 13 commits into from
Oct 8, 2021
Merged

feat(jsii): added a symbol identifier to every type in the assembly #3030

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e79b6ce
feat: added a map from symbol identifiers to fully qualified names in…
Oct 5, 2021
19057de
Fixed tests
Oct 5, 2021
688c58c
Added a symbolId to every type instead of a global map
Oct 6, 2021
9e3ada8
Fixed integration tests
Oct 6, 2021
c02f3a9
Removed leftover prop
Oct 6, 2021
c2f17d0
Update 0.21.2.jsii.json
otaviomacedo Oct 6, 2021
4156836
Fixed snapshot test
Oct 6, 2021
86b6caf
Fixed snapshot test
Oct 6, 2021
635456f
Fixed snapshot tests
Oct 6, 2021
434e5a6
Made test OS agnostic
Oct 6, 2021
8ab42d7
Fixed the symbolIdentifier function to take into account a possible o…
Oct 7, 2021
880bc4e
Splitting the path by either \ or /
Oct 8, 2021
5413e15
Merge branch 'main' into otaviom/symbol-identifiers
mergify[bot] Oct 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/@jsii/spec/lib/assembly.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,11 @@ export interface TypeBase extends Documentable, SourceLocatable {
* The kind of the type.
*/
kind: TypeKind;

/**
* Unique string representation of the corresponding Typescript symbol
*/
symbolId?: string;
}

/**
Expand Down
14 changes: 9 additions & 5 deletions packages/@scope/jsii-calc-base-of-base/test/assembly.jsii
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@
"name": "foo"
}
],
"name": "IVeryBaseInterface"
"name": "IVeryBaseInterface",
"symbolId": "lib/index:IVeryBaseInterface"
},
"@scope/jsii-calc-base-of-base.StaticConsumer": {
"assembly": "@scope/jsii-calc-base-of-base",
Expand Down Expand Up @@ -100,7 +101,8 @@
"variadic": true
}
],
"name": "StaticConsumer"
"name": "StaticConsumer",
"symbolId": "lib/index:StaticConsumer"
},
"@scope/jsii-calc-base-of-base.Very": {
"assembly": "@scope/jsii-calc-base-of-base",
Expand Down Expand Up @@ -132,7 +134,8 @@
}
}
],
"name": "Very"
"name": "Very",
"symbolId": "lib/index:Very"
},
"@scope/jsii-calc-base-of-base.VeryBaseProps": {
"assembly": "@scope/jsii-calc-base-of-base",
Expand All @@ -157,9 +160,10 @@
"fqn": "@scope/jsii-calc-base-of-base.Very"
}
}
]
],
"symbolId": "lib/index:VeryBaseProps"
}
},
"version": "2.1.1",
"fingerprint": "rbtJN11MzD+pqofNrrkTb/GoS1wpG+lpZoOWh+HXysE="
"fingerprint": "6Qes5fbC/YvlKQaK6Oqw0RGgsr/feI/vaRWDYXJJaU8="
}
14 changes: 9 additions & 5 deletions packages/@scope/jsii-calc-base/test/assembly.jsii
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@
}
}
],
"name": "Base"
"name": "Base",
"symbolId": "lib/index:Base"
},
"@scope/jsii-calc-base.BaseProps": {
"assembly": "@scope/jsii-calc-base",
Expand Down Expand Up @@ -141,7 +142,8 @@
"primitive": "string"
}
}
]
],
"symbolId": "lib/index:BaseProps"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intention down the line to leverage the fact that this effectively contains a file path + declaration name?

If so, the relative path might be altered by tsc.outDir... And that might be worth considering?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the deprecation warnings, the only requirement is that this string uniquely identifies the symbol. The idea is: the JSII assembly is the source of truth for deciding whether an element is deprecated. But when we traverse the AST, what we have are Typescript nodes. So this property is the bridge between the two domains. If we can deterministically generate the same strings when writing to the assembly and when reading from it, we can do this mapping.

@rix0rrr do you have other applications in mind?

Copy link
Contributor

@rix0rrr rix0rrr Oct 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good point, I had forgotten about that. The identifier should be stable regardless of whether we're looking at the "uncompiled" source, or the "compiled + distributed" sources that go up on NPM.

These are the potential cases we could be running into:

# Finding symbol in source tree (has original .ts files)
<src_repo>/<package>/src/file.ts

# Finding symbol in a packaged library without an outdir (will have .d.ts and .js files)
.../node_modules/<package>/src/file.d.ts

# Finding symbol in a packaged library WITH an outdir
.../node_modules/<package>/dist/src/file.d.ts

All should ideally have the same identifier, so that we can treat jsii packages that we consume via in-repo symlinks and via monorepo symlinks the same.

That kinda sucks actually :(.

Anyone any good ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean... logically the identifier should have the path src/file in there. We have to think about how to extract that short relative path from this full path though:

.../node_modules/<package>/dist/src/file.d.ts

It requires us to be able to identify the <dist> dir in some way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ho but that's actually not too hard. Given that we're looking for the package.json anyway, that will have outdir set under jsii.tscOptions (or whatever the configuration is again). So it would work something like this:

function determineRelativeFilename(fullPath: string) {
  const packageJsonDir = findPackageJson(fullPath);
  const pj = loadPackageJson(packageJsonDir);

  const root = pj.jsii?.tscOptions?.outdir ? path.join(packageJsonDir, pj.jsii?.tscOptions?.outdir) : packageJsonDir;

  return stripExtension(path.relative(root, fullPath));
}

},
"@scope/jsii-calc-base.IBaseInterface": {
"assembly": "@scope/jsii-calc-base",
Expand All @@ -164,7 +166,8 @@
"name": "bar"
}
],
"name": "IBaseInterface"
"name": "IBaseInterface",
"symbolId": "lib/index:IBaseInterface"
},
"@scope/jsii-calc-base.StaticConsumer": {
"assembly": "@scope/jsii-calc-base",
Expand Down Expand Up @@ -198,9 +201,10 @@
"variadic": true
}
],
"name": "StaticConsumer"
"name": "StaticConsumer",
"symbolId": "lib/index:StaticConsumer"
}
},
"version": "0.0.0",
"fingerprint": "rIJAOTzUs2OYke2UCJzexq/fqJOiqmR9PbTMlf37o/A="
"fingerprint": "DVCANvLLzJEu5VNOJVmldbT5wTXhmHlZzk3E6muTR5I="
}
56 changes: 37 additions & 19 deletions packages/@scope/jsii-calc-lib/test/assembly.jsii
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,8 @@
]
}
],
"name": "BaseFor2647"
"name": "BaseFor2647",
"symbolId": "lib/index:BaseFor2647"
},
"@scope/jsii-calc-lib.DiamondLeft": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -237,7 +238,8 @@
"primitive": "number"
}
}
]
],
"symbolId": "lib/duplicate-inherited-prop:DiamondLeft"
},
"@scope/jsii-calc-lib.DiamondRight": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -285,7 +287,8 @@
"primitive": "boolean"
}
}
]
],
"symbolId": "lib/duplicate-inherited-prop:DiamondRight"
},
"@scope/jsii-calc-lib.EnumFromScopedModule": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -314,7 +317,8 @@
"name": "VALUE2"
}
],
"name": "EnumFromScopedModule"
"name": "EnumFromScopedModule",
"symbolId": "lib/index:EnumFromScopedModule"
},
"@scope/jsii-calc-lib.IDoublable": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -345,7 +349,8 @@
"primitive": "number"
}
}
]
],
"symbolId": "lib/index:IDoublable"
},
"@scope/jsii-calc-lib.IFriendly": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -379,7 +384,8 @@
}
}
],
"name": "IFriendly"
"name": "IFriendly",
"symbolId": "lib/index:IFriendly"
},
"@scope/jsii-calc-lib.IThreeLevelsInterface": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -410,7 +416,8 @@
"name": "baz"
}
],
"name": "IThreeLevelsInterface"
"name": "IThreeLevelsInterface",
"symbolId": "lib/index:IThreeLevelsInterface"
},
"@scope/jsii-calc-lib.MyFirstStruct": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -480,7 +487,8 @@
}
}
}
]
],
"symbolId": "lib/index:MyFirstStruct"
},
"@scope/jsii-calc-lib.Number": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -553,7 +561,8 @@
"primitive": "number"
}
}
]
],
"symbolId": "lib/index:Number"
},
"@scope/jsii-calc-lib.NumericValue": {
"abstract": true,
Expand Down Expand Up @@ -606,7 +615,8 @@
"primitive": "number"
}
}
]
],
"symbolId": "lib/index:NumericValue"
},
"@scope/jsii-calc-lib.Operation": {
"abstract": true,
Expand Down Expand Up @@ -643,7 +653,8 @@
}
}
],
"name": "Operation"
"name": "Operation",
"symbolId": "lib/index:Operation"
},
"@scope/jsii-calc-lib.StructWithOnlyOptionals": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -709,7 +720,8 @@
"primitive": "boolean"
}
}
]
],
"symbolId": "lib/index:StructWithOnlyOptionals"
},
"@scope/jsii-calc-lib.submodule.IReflectable": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -745,7 +757,8 @@
}
}
}
]
],
"symbolId": "lib/submodule/index:IReflectable"
},
"@scope/jsii-calc-lib.submodule.NestingClass": {
"assembly": "@scope/jsii-calc-lib",
Expand All @@ -760,7 +773,8 @@
"line": 22
},
"name": "NestingClass",
"namespace": "submodule"
"namespace": "submodule",
"symbolId": "lib/submodule/index:NestingClass"
},
"@scope/jsii-calc-lib.submodule.NestingClass.NestedClass": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -796,7 +810,8 @@
"primitive": "string"
}
}
]
],
"symbolId": "lib/submodule/index:NestedClass"
},
"@scope/jsii-calc-lib.submodule.NestingClass.NestedStruct": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -830,7 +845,8 @@
"primitive": "string"
}
}
]
],
"symbolId": "lib/submodule/index:NestedStruct"
},
"@scope/jsii-calc-lib.submodule.ReflectableEntry": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -877,7 +893,8 @@
"primitive": "any"
}
}
]
],
"symbolId": "lib/submodule/index:ReflectableEntry"
},
"@scope/jsii-calc-lib.submodule.Reflector": {
"assembly": "@scope/jsii-calc-lib",
Expand Down Expand Up @@ -926,9 +943,10 @@
}
],
"name": "Reflector",
"namespace": "submodule"
"namespace": "submodule",
"symbolId": "lib/submodule/index:Reflector"
}
},
"version": "0.0.0",
"fingerprint": "yaW4nEkJcW4ZyHTJGV9YSAtkrzeBBoXkDffMJBD8dGQ="
"fingerprint": "BXEo4aMVmNYUV0dd8hK2zOYLM8iFbpxQbUzGyGagFu8="
}
Loading