-
Notifications
You must be signed in to change notification settings - Fork 705
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
Fixed skipping of nested object literals with --excludeNotExported #1103
Conversation
…excludeNotExported
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.
Thanks for adding the tests to ensure this doesn't break in the future, I'm a bit worried about the mechanism used to ensure they are exported though...
} else if (container.kindOf([ReflectionKind.TypeLiteral]) && context.converter.excludeNotExported) { | ||
// If the container for this is a type literal, it never had the | ||
// isExported flag set on it. See https://github.com/TypeStrong/typedoc/issues/953 | ||
isExported = true; |
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 worries me. Is there a reason we can't set the isExported
flag on containers when creating them instead of 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.
Yeah, I looked into that. That does seem like the more "correct" way to go about it. However, after extensive debugging I discovered that this factory function doesn't ever get called for object literals themselves. Instead, I believe the associated Declaration is created here: https://github.com/TypeStrong/typedoc/blob/master/src/lib/converter/types/reference.ts#L127
Perhaps there is an underlying issue where object literal declarations follow a different code path. I didn't want to go there, so I just wrote some tests to determine the least invasive fix, which is what you see.
Happy to dive back in if you can provide some additional insights about code structure. It's a pretty challenging codebase. :)
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, thanks. I would like to fix this the right way, but haven't had enough time to dig in and see where that would be yet, so unfortunately I can't provide any guidance here. Maybe next weekend.
Yes, the codebase is rather difficult to follow, we're working on it, but its slow going. :)
I spent some time digging in to this and was able to mostly fix it by inheriting the |
OK, makes sense. Thanks for looking into it. Happy to help with that one where I can. |
@Gerrit0 I pulled #801 and added my test to it. The main converter test (with my added |
Good to know, thanks. I'll be sure to pull your test in when I look into this this weekend. |
Also: - clean up scripts/rebuild_specs.js, making it easier to use for debugging a single test. - move log message about TS version to the CLI - Use rimraf instead of rm for Windows compatibility when developing - Fix bugs with object literals causing a crash, rebuild tests.
Sorry about the delay here! I've confirmed that this is fixed in 0.16.0-8 ( |
Should fix #901, #953 and #958.