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.

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

Support linking to module.d.ts #1117

Merged
merged 23 commits into from
Oct 27, 2019
Merged

Conversation

PissedCapslock
Copy link
Contributor

See my question on https://stackoverflow.com/q/58429798/1076463

Creating a link from firstmodule.d.ts file to the module secondmodule.d.ts is impossible:

  • The name of the second module that you need to use is "secondmodule.d", e.g. {@link "secondmodule.d"} (including the quotes)
  • However, the name gets split on the ., so in the ProjectReflection#findReflectionByName, you end up comparing "secondmodule.d" with just d due to the const names: string[] = Array.isArray(arg) ? arg : arg.split('.'); line

This proposed change strips the .d from the name, allowing you to use {@link "secondmodule"}.

Ideally I would also strip the quotes, but that causes a lot of test failures.
My proposed change seems to pass all existing tests, although it affects the json file that gets generated. This sounds like a backwards incompatible change, but no idea how strict the project is on this.

I also haven't quite figured out how to add a test for this :-(

If this PR is not accepted because you don't want to change the outputted json, can you tell me (or answer on SO) how to create that link.

See my question on https://stackoverflow.com/q/58429798/1076463

Creating a link from `firstmodule.d.ts` file to the module `secondmodule.d.ts` is impossible:

- The name of the second module that you need to use is `"secondmodule.d"`, e.g. `{@link "secondmodule.d"}` (including the quotes)
- However, the name gets split on the `.`, so in the `ProjectReflection#findReflectionByName`, you end up comparing `"secondmodule.d"` with just `d` due to the `const names: string[] = Array.isArray(arg) ? arg : arg.split('.');` line

This proposed change strips the `.d` from the name, allowing you to use `{@link "secondmodule"}`.

Ideally I would also strip the quotes, but that causes a lot of test failures.
My proposed change seems to pass all existing tests, although it affects the json file that gets generated. This sounds like a backwards incompatible change, but no idea how strict the project is on this.

I also haven't quite figured out how to add a test for this :-(

If this PR is not accepted because you don't want to change the outputted json, can you tell me (or answer on SO) how to create that link.
Fixing linting problems
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Oct 21, 2019

Hmm, renaming declaration files doesn't seem like a good idea to me, it increases the likelihood of duplicate entries / entries that get confused with each other.

What do you think about changing the .split('.') to a custom function so the string doesn't get split when within quotes?

@PissedCapslock
Copy link
Contributor Author

That sounds like it would fix my problem as well

@PissedCapslock
Copy link
Contributor Author

@Gerrit0 I have changed the PR to use a custom split function. The only thing I am not happy about is the test that I added. To have access to the new utility method in the test, I needed to export it.

I have no idea whether there is a better way to fix this. In Java (which am I much more familiar with), I would have used a package private static method and put the test in the same package. This would give the test access to the static method, and avoid that it shows up in the API.

If you have any good suggestions on how to fix that, please share them (or simply adjust my PR).

Typo
See my question on https://stackoverflow.com/q/58429798/1076463

Creating a link from `firstmodule.d.ts` file to the module `secondmodule.d.ts` is impossible:

- The name of the second module that you need to use is `"secondmodule.d"`, e.g. `{@link "secondmodule.d"}` (including the quotes)
- However, the name gets split on the `.`, so in the `ProjectReflection#findReflectionByName`, you end up comparing `"secondmodule.d"` with just `d` due to the `const names: string[] = Array.isArray(arg) ? arg : arg.split('.');` line

This proposed change strips the `.d` from the name, allowing you to use `{@link "secondmodule"}`.

Ideally I would also strip the quotes, but that causes a lot of test failures.
My proposed change seems to pass all existing tests, although it affects the json file that gets generated. This sounds like a backwards incompatible change, but no idea how strict the project is on this.

I also haven't quite figured out how to add a test for this :-(

If this PR is not accepted because you don't want to change the outputted json, can you tell me (or answer on SO) how to create that link.
Fixing linting problems
@PissedCapslock
Copy link
Contributor Author

Hmm, I am relatively new to Github. Tried to rebase my PR to the latest state of master, but that was perhaps not my best idea. Suddenly it looks like I touched a lot of files.

@Gerrit0 Gerrit0 merged commit cc4cef1 into TypeStrong:master Oct 27, 2019
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Oct 27, 2019

Rebasing when the same file has been changed with git can easily go wrong. I usually merge when pulling in new changes.

Its kind of weird that avoiding the split only works in earlier quoted members (splitUnquotedString('1."2.3".4', '.') // => [ '1', '"2', '3", '4']), but this actually ends up preserving existing behavior around quotes, so I think its fine. Eventually we should either adopt TSDoc for parsing comments, or come up with a spec that deals with edge cases like members having dots in their name ("fixing" it so that the above example returns ['1', '"2.3"', '4'] wouldn't do it, if the property name was 2.3 without quotes)

I think this is an improvement on existing behavior, and everything seems to work locally.

Thanks for the contribution!

@PissedCapslock
Copy link
Contributor Author

Thanks for accepting it.
Did you had any suggestions about that test ? Because now we added an extra export which will show up in the autocomplete of people's IDEs.

If this was Java (the language I am most familiar with), I would have used:

  • Either a package visible static method to do that splitting, or put it in an internal class which does not show up in the API (or at least, only in an obfuscated way screaming "do not use this").
  • Put the test in the same package as the class so that it has access to the static method

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Oct 27, 2019

TypeDoc's exported API surface is already badly curated, so I wasn't too worried about adding an extra method to it yet. Ideally, yes, it would be best to not export it on the root level.

In the test I'd probably just import it using the full path to the utils file.

It's on the list to go through what's exported and what isn't and figure out what should actually be exported. The Component decorator currently isn't exported, but it is used by almost every single plugin for example.

@Gerrit0 Gerrit0 added this to the 0.16.0 milestone Nov 3, 2019
@Gerrit0 Gerrit0 removed this from the 0.16.0 milestone Nov 13, 2019
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Nov 13, 2019

This has been released in 0.15.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants