-
-
Notifications
You must be signed in to change notification settings - Fork 737
Support for monorepos based on npm/Yarn Workspaces #1567
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
Conversation
Based on npm / Yarn workspaces, introducing the --packages option
src/lib/converter/converter.ts
Outdated
@@ -19,6 +19,12 @@ import { IMinimatch } from "minimatch"; | |||
import { hasAllFlags, hasAnyFlag } from "../utils/enum"; | |||
import { resolveAliasedSymbol } from "./utils/symbols"; | |||
|
|||
export interface PackageProgram { |
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'm not that happy with the name PackageProgram
for this concept, any suggestions?
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.
DocumentationEntrypoint
perhaps? This is a hard thing to name...
src/lib/converter/converter.ts
Outdated
@@ -134,11 +140,13 @@ export class Converter extends ChildableComponent< | |||
* Compile the given source files and create a project reflection for them. | |||
* | |||
* @param entryPoints the entry points of this program. | |||
* @param program the program to document that has already been type checked. | |||
* @param programs the programs to document, that have already been type checked. | |||
* @param packages an array of packages (used in --packages mode) | |||
*/ | |||
convert( | |||
entryPoints: readonly string[], |
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'm not that happy with the way in which I've plumbed the packages as a third param to convert where you're basically either using the first 2 params or the third. However I wasn't really sure if it would make sense to support passing non-package entry-points as well as packages simultaneously in to this system, or if these data structures could be unified earlier in the processing to make this less weird.
Looking for ideas here on better architecture.
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 think the way I'd like to handle this is to make a breaking change, and accept an array of DocumentationEntrypoint
, then the caller is responsible for turning an array of programs (in the entryPoint case) or packages into the appropriate structure.
Means this can't be released without a minor version bump, but I think this feature deserves one anyways - I'll take what I've done towards 0.21, merge it with this, and call the theme refactor 0.22 rather than making this wait.
src/lib/converter/converter.ts
Outdated
entryPoints: readonly string[], | ||
context: Context, | ||
packages: PackageProgram[] | ||
) { | ||
const baseDir = getCommonDirectory(entryPoints); | ||
const entries: { |
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.
We do manage to unify the different sources of program / entry point in to the entries
here.
src/lib/converter/converter.ts
Outdated
@@ -269,7 +282,12 @@ export class Converter extends ChildableComponent< | |||
for (const program of context.programs) { | |||
const sourceFile = program.getSourceFile(file); | |||
if (sourceFile) { | |||
entries.push({ file, sourceFile, program }); | |||
entries.push({ | |||
entryName: getModuleName(resolve(file), baseDir), |
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.
getModuleName
logic just moved up from below
for (const entry of entries) { | ||
context.setActiveProgram(entry.program); | ||
entry.context = this.convertExports( | ||
context, | ||
entry.sourceFile, | ||
entryPoints, | ||
getModuleName(resolve(entry.file), baseDir) | ||
entries.length === 1, |
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.
Moving entries.length === 1
up from below wasn't necessary, I had some potential changes in this code which I think I didn't need in the end, but left this as I found it simpler to understand than seeing the whole array passed.
const builder = new NavigationBuilder( | ||
project, | ||
project, | ||
this.application.options.getValue("entryPoints").length > 1 |
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.
"entryPoints" option no longer matches the number of modules we're documenting so I needed a change here. I think using project.getReflectionsByKind(ReflectionKind.Module).length
is valid, but that might not be the number of modules we're going to actually document, so please let me know if this needs more investigation.
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 does change the logic a bit, and unfortunately is considerably more expensive since it loops over every reflection in the project (both direct children and their children). A better check would be project.getChildrenByKind(ReflectionKind.Module).length !== 0
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 suggestion seems to work 👍
src/lib/utils/package-manifest.ts
Outdated
* @param obj the receiver of the hasOwnProperty method call | ||
* @param prop the property to test for | ||
*/ | ||
function hasOwnProperty<X extends {}, Y extends PropertyKey>( |
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 very generic utility function but I wasn't sure if it should live elsewhere.
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.
Here is fine, if something else needs it it can always be moved.
@@ -0,0 +1 @@ | |||
docs |
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.
Running typedoc on the examples folder locally, I needed to gitignore this folder.
Sorry I haven't had the time to look at this yet! It looks like a really neat idea. I should be able to look at it next weekend. |
@Gerrit0 thanks for letting me know you saw this. Of course, take however long you wish. |
This is super useful, can we get this supported in next release? |
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.
Looking good!
bin/typedoc
Outdated
if ( | ||
app.options.getValue("entryPoints").length === 0 && | ||
app.options.getValue("packages").length === 0 | ||
) { | ||
app.logger.error("No entry points provided"); |
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'd like to also mention packages here
src/lib/converter/converter.ts
Outdated
@@ -19,6 +19,12 @@ import { IMinimatch } from "minimatch"; | |||
import { hasAllFlags, hasAnyFlag } from "../utils/enum"; | |||
import { resolveAliasedSymbol } from "./utils/symbols"; | |||
|
|||
export interface PackageProgram { |
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.
DocumentationEntrypoint
perhaps? This is a hard thing to name...
src/lib/converter/converter.ts
Outdated
@@ -134,11 +140,13 @@ export class Converter extends ChildableComponent< | |||
* Compile the given source files and create a project reflection for them. | |||
* | |||
* @param entryPoints the entry points of this program. | |||
* @param program the program to document that has already been type checked. | |||
* @param programs the programs to document, that have already been type checked. | |||
* @param packages an array of packages (used in --packages mode) | |||
*/ | |||
convert( | |||
entryPoints: readonly string[], |
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 think the way I'd like to handle this is to make a breaking change, and accept an array of DocumentationEntrypoint
, then the caller is responsible for turning an array of programs (in the entryPoint case) or packages into the appropriate structure.
Means this can't be released without a minor version bump, but I think this feature deserves one anyways - I'll take what I've done towards 0.21, merge it with this, and call the theme refactor 0.22 rather than making this wait.
const builder = new NavigationBuilder( | ||
project, | ||
project, | ||
this.application.options.getValue("entryPoints").length > 1 |
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 does change the logic a bit, and unfortunately is considerably more expensive since it loops over every reflection in the project (both direct children and their children). A better check would be project.getChildrenByKind(ReflectionKind.Module).length !== 0
src/lib/utils/package-manifest.ts
Outdated
* @param obj the receiver of the hasOwnProperty method call | ||
* @param prop the property to test for | ||
*/ | ||
function hasOwnProperty<X extends {}, Y extends PropertyKey>( |
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.
Here is fine, if something else needs it it can always be moved.
src/lib/utils/package-manifest.ts
Outdated
typeof packageJSON.workspaces === "object" && | ||
packageJSON.workspaces != null && | ||
hasOwnProperty(packageJSON.workspaces, "packages") && | ||
Array.isArray(packageJSON.workspaces.packages) |
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.
Here too
* Finds the corresponding TS file from a transpiled JS file. | ||
* The JS must be built with sourcemaps. | ||
*/ | ||
function getTsSourceFromJsSource( |
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 doesn't handle inline source maps properly... probably fine to ignore that as a first pass.
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.
So i was thinking of pulling in a proper sourcemap lib, rather than the quick homegrown approach I took, which would presumably handle all kinds of things better than i have.|
Unfortunately they tended to be asynchronous (rightly so), which would likely mean re-working some of typedoc's apis to be async, which is probably warranted but felt like a big change.
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.
Yeah, TypeDoc's APIs are slowly becoming async... but it's happening on an as-needed basis, so if something doesn't require async yet, it doesn't use it. I think we just ignore this edge case for now.
src/lib/utils/package-manifest.ts
Outdated
): string | undefined { | ||
const contents = readFile(jsPath); | ||
const sourceMapPrefix = "//# sourceMappingURL="; | ||
const searchResult = contents.search( |
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.
Am I missing some reason contents.indexOf("\n//# sourceMappingURL=")
doesn't work?
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.
Seems like it would work.
} | ||
// There's a pretty large assumption in here that we only have | ||
// 1 source file per js file. This is a pretty standard typescript approach, | ||
// but people might do interesting things with transpilation that could break this. |
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 don't think I care - at least for a first pass.
packageJsonPath, | ||
packageJson | ||
); | ||
if (packageEntryPoint === undefined) { |
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.
Question - why is failing to find an entry point sometimes a fatal error and sometimes just a warning? Is one reason for failure more common/expected?
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.
My razor here would be:
Ignore things which don't appear to be require
-able node packages.
Fail on things which appear to be require
-able node packages but are missing the necessary metadata for us to document.
I'll actually add the above as a comment in the code to guide maintainers.
The scenario which I saw so far, was just from running this across one of my own codebases where some of my packages were actually webpack browser bundles, not intended to be consumed as node.js modules, so there is no js entry point to be seen. This package for example: https://github.com/efokschaner/laniakea/tree/master/demos/balls/client
I fully anticipate we're going to see some surprising things in the wild w.r.t to packages and package structure for which this first pass at some default behaviour will not suffice. This comes back to my general remark above about potentially needing support for per-package typedoc config in the future, when we learn what people might need to tune.
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.
Makes sense 👍
Per-package configuration might be difficult... excludeExternals
for example changes the theme output... future work.
I don't plan on holding this up once it's ready, will release as 0.21 and push the theme rework to 0.22. |
For tests - the way I'd like to handle this is a couple small monorepo style projects under src/test/packages and either snapshotting the JSON like the converter tests do, or spot checking like the converter2 tests do. The examples folder should really have never been used for tests, I just haven't moved them out of that folder yet. |
let source = sourceMap.sources[0]; | ||
// If we have a sourceRoot, trim any leading slash from the source, and join them | ||
// Similar to how it's done at https://github.com/mozilla/source-map/blob/58819f09018d56ef84dc41ba9c93f554e0645169/lib/util.js#L412 | ||
if (sourceRoot !== undefined) { |
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.
Source maps don't have to contain a sourceRoot and we handle that now. I also got this a little more conformant with the spec.
// We can use require.resolve to let node do its magic. | ||
// Pass an empty `paths` as node_modules locations do not need to be examined | ||
try { | ||
jsEntryPointPath = require.resolve(jsEntryPointPath, { paths: [] }); |
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 the other change in here. package.json's main
values seem to be roughly as flexible as the require()
paths themselves in terms of what actual file gets loaded.
@@ -67,7 +67,7 @@ | |||
], | |||
"scripts": { | |||
"pretest": "node scripts/copy_test_files.js", | |||
"test": "nyc --reporter=html --reporter=text-summary mocha --timeout=10000 'dist/test/**/*.test.js'", | |||
"test": "nyc --reporter=html --reporter=text-summary mocha --timeout=10000 'dist/test/**/*.test.js' --exclude 'dist/test/packages/**'", |
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.
Because the /packages/
folder contains a non-trivial amount of sample code we need to avoid accidentally picking up tests in there
try { | ||
// The "packages" option is an array of paths and should be interpreted as relative to the typedoc.json |
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 came up in testing because the tests cwd is not inside the project we're operating on, and without this the paths in the typedoc.json are interpreted relative to cwd, rather than to the file in which they appear.
This actually seems like something you might want to change for all options which are filepaths but could appear in the typedoc.json.
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.
Yes! I completely agree, #1442 tracks this, but last time I went after it the approach I took failed.
); | ||
if (npmMajorVer < 7) { | ||
throw new Error( | ||
"npm version must be at least 7, version installed is " + |
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.
Installing the monorepo's deps (and gluing the inter-package deps together) requires either lerna, or yarn, or npm>7. I opted for the npm approach as I figured it's the most "vanilla" despite having been around for the least amount of time. This could run in to issues under some of your CI configs however so let's see if anything breaks and decide what to do.
"example" | ||
], | ||
"author": "", | ||
"license": "Apache-2.0", |
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 copied the same license as typedoc itself, not that it should matter much for this little island of code.
"test": "node ." | ||
}, | ||
"devDependencies": { | ||
"typescript": "^4.2.4" |
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.
Technically this package should depend on typedoc? I never had to install it because the tests bring their own typedoc.
It gets a bit weird though as if treated in isolation this would pull in typedoc from the internet. What you actually have here is a monorepo XD where you want packages developed in the same source tree to use each other's development copies.
I recommend reviewing the newest changes commit by commit. I was fairly happy with the outcome of your DocumentationEntrypoint suggestions, in this commit 23db9a3 In 998e610 I took a slightly elaborate approach to the testing, but one that gave me pretty good results and already lead to me improving the new behaviour in a couple of ways. Let me know if this test approach seems silly. I went looking for good examples of a skeleton of monorepo setup and I found this repo to be a well built example: https://github.com/NiGhTTraX/ts-monorepo . Rather than vendor that in to the tests, or embroil myself in git submodules, I decided to clone the repo during I actually forked the repo and added typedoc support (efokschaner/ts-monorepo@49a565e), and so the tests pull from my fork (with a pinned commit sha). You can see the docs typedoc currently generates here (https://efokschaner.github.io/ts-monorepo/) I was actually considering approaching the author of Let me know how things look, what else to change. As we get a little closer to merging this feature, I was wondering if it would make sense to pitch the monorepo support as a little more experimental than the rest of the typedoc featureset, in the readme/docs? Just to manage expectations as it's likely to discover a fair bit of complexity. PS. Github workflows just introduced "approval for first-timers", which is now pending on this PR. |
ok(project, "Failed to convert"); | ||
const result = app.serializer.projectToObject(project); | ||
ok(result.children !== undefined); | ||
strictEqual( |
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.
The validation I'm doing in this and other cases is pretty minimal atm, I mainly wanted to focus on the new behaviour around discovering package entrypoints as I assume most other behaviour is already tested elsewhere. LMK if there are other things you believe we should check here though.
I need to pull this down and play with it some more, but overall I'm really happy with this. I think I'll probably pull the monorepo tests into this repo rather than downloading them, since keeping tests fast is really important to me (TypeDoc's tests used to take 3 minutes to run, which pretty much meant they were useless for quickly verifying that everything worked as expected... getting it down much lower was a huge productivity boost) |
If you're looking to try this out - it has been published under 0.21.0-beta.2 |
I gave this a shot, and I'm having an issue with my particular monorepo. It is a PNPM setup, but manually pointing Is this an unsupported edge case? |
Hey @Monkatraz, it's not a use case I'd considered, but it sounds pretty easy to support. I can push another commit to this PR or we can do a separate PR based on @Gerrit0 's preference. |
@Gerrit0 regarding your comment about vendoring the monorepo code, that probably won't significantly improve the time to bootstrap the tests as the clone is outweighed by the time to install deps and build that repo, which is what's required to have the js generated for us to have sourcemaps to find the TS entry points. I agree with your objective to keep the test cycle fast of course. One solution to that could be to not wipe+install+rebuild the codebase used in the tests each time the full test script is called. This has some downsides in that the first test run still takes the full time, and the test run is no longer from a completely clean state by default. The solution I used to this locally was to just separate my use of test setup and test iteration. I ran the npm test command once to get the copy_test_files pretest setup done once and then just ran the nyc mocha command directly to iterate without all the re-setup. So I intentionally opted in to the not-from-scratch test iteration workflow. So I could see 3 possible approaches here:
Do you have other options in mind? What do you think makes most sense? |
I've noticed that assumed base directory for packages is a location of the TypeDoc config (which is different from entryPoints approach), shouldn't it be uniform with current behaviour until it can be changed simultaneously? I agree with Monkatraz, we should get support for Also, is there any reason for defining edit: Found solutions to both matters |
Hey @lhoerion , regarding the paths relative to typedoc.json:
I can see the reasoning but given that this is an entirely new config option, unlikely to be used simultaneously with the entryPoints option, and that the way this one works is more likely to match people's default assumptions about how a path in a config file works, I'd advocate it do it the better way from the outset. Can you elaborate on how you see the package json
|
If someone wish to document type definitions package, then usually they won't have
True, I'm also in favour of leaving it as-is, but we've to document this difference then. |
When I pulled this into the beta branch, what I ended up doing was pulling the project into this repo and also committing built files. The additional dependencies didn't seem to add any value, so the tests are very small now.
I like the behavior it has - and have wanted to change the rest of the options that have paths to use this same behavior, just haven't gotten around to it yet... going to try it this morning. |
@Gerrit0 thanks for your support integrating this code and getting the tests in to a good state. I just wanted to check if you have any expectations from me about anything remaining to fix or change for this PR. If not, feel free to close this PR. If we were to implement @Monkatraz 's use case here #1567 (comment) should I make that PR to the beta branch? |
I think that my PR covered that case as well |
@lhoerion ah yes! Ty. Does it work to test for the file being .ts after it's passed to require.resolve? |
Yes it works, that way .ts/.js extension doesn't need to be explicitly provided in |
Nothing else from me! I've been leaving it open to remind myself to give you lots of thanks in the release notes for 0.21. |
Hi @efokschaner I gave the feature a try, I find it doesn't support the inline source option in getTsSourceFromJsSource(). I got no such file exception because it tries to open the encoded source code like: data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiaW5kZXguanMiLCJzb3VyY2VSb290IjoiIiwic291cmNlcyI6WyIuLi8uLi9zcmMvaW5kZXgudHMiXSwibmFtZXMiOltdLCJtYXBwaW5ncyI6Ijs7O0FBQUEsNERBQWtDO0FBQ2xDLHdEQUE4QiIsInNvdXJjZXNDb250ZW50IjpbImV4cG9ydCAqIGZyb20gXCIuL0Fib3J0Q29udHJvbGxlclwiO1xuZXhwb3J0ICogZnJvbSBcIi4vQWJvcnRTaWduYWxcIjtcbiJdfQ== Another question is how to show the README page of sub packages. Thank again for your work! |
Hi @AllanZhengYP, in typedoc/src/lib/utils/package-manifest.ts Line 137 in 90cd4a0
|
This has been fully released in 0.21.0, thanks again! |
As mentioned in #1548 , and due to the fact that the previous solution to this stopped working with typedoc 0.20 I wanted to see if I could implement support for npm / Yarn Workspaces.
It still needs some automated tests as well as some design direction, but I just wanted to show how it looks, get the maintainers' initial thoughts, and see if this is a feature / mode that typedoc would be interested in having. (If not, I'd really appreciate guidance on how the same thing could be pulled off as a plugin?)
See the README for an overview of how this mode works. You can also see this version in action in the docs for this monorepo: https://efokschaner.github.io/laniakea/index.html
I'll leave some comments around the PR for some specific things I'd like to discuss.
Contrast with classic "entrypoint" style invocation
When passing
--packages
rather than entry points, we only document the single "main" entry point for each package found. This means we're only documenting the package's external api, and not all the modules within the package forming its implementation. This may be desirable, but some may still prefer to document each inner module within their packages. This also means currently we wouldn't support packages with multiple entry points (the kind which are designed for import/require using"packagename/submodule"
syntax).In addition, we only use the default "implicit" tsconfig discovery process for each package (
ts.findConfigFile
). This is an assumption that the package's tsc invocation isn't doing anything too fancy with its command line params or the location / name of the tsconfig.There seems to be a theme of potentially needing configuration at the per-package level in order to deal with cases where people would need or want non-default behaviour. I could see a world perhaps where we eventually introduce support for a typedoc.json at the level of each individual package to deal with these kinds of things.
Inter-package references
One of the current limitations is that in this mode, typedoc sees inter-package references just like tsc does, as references to type declarations, rather than to the original source.
For example, see this reference to
NominalType
where it says it's defined inutils/dist/nominal-type.d.ts:5
rather than connecting it to the original source as it does herevscode has had similar issues to this in the past with "Go to definition" taking you to .d.ts files (eg. microsoft/vscode#19672). Eventually this was solved via "declaration maps" and potentially the same thing could be used here to connect the inter-package references back to the original source by having typedoc use the declaration maps.
Tests
If there's interest in integrating this feature, I'm wondering what kind of tests would be preferred? My instinct is to begin with a few happy-path end-to-end regression tests, based on a couple of new sample projects in the examples folder to demonstrate the different ways one can use
--packages
, perhaps using the json converter output format to freeze the expected results?