-
Notifications
You must be signed in to change notification settings - Fork 246
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(jsii): added a symbol identifier to every type in the assembly #3030
Conversation
… the assembly This will be useful for the generation of deprecation warnings and to improve the Rosetta samples.
@@ -141,7 +142,8 @@ | |||
"primitive": "string" | |||
} | |||
} | |||
] | |||
], | |||
"symbolId": "lib/index:BaseProps" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny concern around path handling on Windows, otherwise love it!
packages/jsii/lib/utils.ts
Outdated
} | ||
|
||
function removePrefix(prefix: string, filePath: string) { | ||
const prefixParts = prefix.split(path.sep); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit dangerous. On Windows, the /
will work fine (and so people might use it) but it's not in path.sep
, and so the splitting will behave differently.
It's excellent that you thought of this, because it will be an issue. For safety, can we split on /\/|\\/g
instead? (I know it looks daft but it's splitting on either \
or /
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Thanks!
The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged. |
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
Merging (with squash)... |
Merging (with squash)... |
Symbol identifiers follow the pattern
<relative file path without extension>:<scope names>/<symbol name>
This will be useful for the generation of deprecation warnings and to improve the Rosetta samples.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.