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

types(config): add GlobalConfig interface #6247

Merged
merged 6 commits into from Apr 29, 2021

Conversation

jakovljevic-mladen
Copy link
Member

Description:
The reason why I wanted to add an interface for config object is so that we can have a proper documentation page for it. Constant documentation pages don't have a list of properties like interface documentation pages do.
Also, by introducing this interface, we will fix an error while building the docs (npm run docs) from this line:

* use the {@link onUnhandledError} configuration option or use a runtime handler (like `window.onerror` or

because a link to onUnhandledError will now be available.

I was thinking about naming this interface with just as Config, but I chose GlobalConfig in the end. If needed, I can change to Config if it feels more appropriate.

Related issue (if exists):
None

@benlesh
Copy link
Member

benlesh commented Apr 22, 2021

@niklas-wortmann it seems odd that we have to introduce a type we're going to publish to the masses just to get documentation here. Is there a better way to fix this?

@benlesh
Copy link
Member

benlesh commented Apr 22, 2021

I guess a counter argument is that we're introducing a type either way. 🤔 I'm on the fence here. @cartant? @kwonoj?

@cartant
Copy link
Collaborator

cartant commented Apr 22, 2021

I think this is fine, but I don't see why it needs to be in types.ts and I don't see why it needs to be exported. Does the docs generation really require the latter?

@jakovljevic-mladen
Copy link
Member Author

Does the docs generation really require the latter?

@cartant I think so. When I was working on #6233, I couldn't make AjaxDirection show on the list up until I exported it.

@cartant
Copy link
Collaborator

cartant commented Apr 22, 2021

... it seems odd that we have to introduce a type we're going to publish to the masses just to get documentation here.

I'm more surprised that the type has to be exported, TBH.

@jakovljevic-mladen
Copy link
Member Author

If I move GlobalConfig to config.ts file (with or without exporting the interface), I get this error: Error in {@link GlobalConfig} - Invalid link (does not match any doc): "GlobalConfig" - doc "index/config". This is basically this line failing.

@cartant
Copy link
Collaborator

cartant commented Apr 22, 2021

My preference would be to try to figure out why the docs generation requires this, as, IIRC, it's legal to use parameters (for example) in an exported API that have a type that is not exported - the type alias is expanded. TBH, I think this is done in several places, so I guess those doc's don't work either?

@jakovljevic-mladen
Copy link
Member Author

I could try playing with the docs builder to see why it fails to collect stuff that are not exported from internal/types.ts. Maybe it just needs to be exported, but if we export it in another file (and not export it publicly), it might work... Not sure though...

However, there could be another solution to this, but it would take time to be handled and I'm not even sure it could be handled at all. I could try to create a new template just for this case where that new template would have a list of properties much like a template for interfaces has.

Would that be a good idea? 🤔

BTW, this is how this looks like now (on master, without changes from this PR):
image
At least I managed to add some types instead of any.

@jakovljevic-mladen
Copy link
Member Author

My preference would be to try to figure out why the docs generation requires this

I'll try.

IIRC, it's legal to use parameters (for example) in an exported API that have a type that is not exported - the type alias is expanded. TBH, I think this is done in several places, so I guess those doc's don't work either?

Do you have an example of this?

@cartant
Copy link
Collaborator

cartant commented Apr 22, 2021

Do you have an example of this?

Pretty much all of the config types, IIRC:

https://github.com/ReactiveX/rxjs/blob/7.0.0-rc.2/src/internal/operators/share.ts#L7

@jakovljevic-mladen
Copy link
Member Author

ShareConfig doesn't seem to be shown anywhere:

  • image
  • image

@cartant
Copy link
Collaborator

cartant commented Apr 22, 2021

ShareConfig doesn't seem to be shown anywhere

Yeah, that's why I think we should try to fix the docs generation. (FWIW, I thought TS had an issue with interfaces - rather than type aliases - being used without being exported, but I guess not.)

@jakovljevic-mladen
Copy link
Member Author

Yeah, that's why I think we should try to fix the docs generation

I'll do my best, but I think we'll need help from @niklas-wortmann for this one.

@cartant
Copy link
Collaborator

cartant commented Apr 23, 2021

I'm inclined to change my mind. I think introducing and exporting the type is a small price to pay and we should make sure that all of the other types that are used for parameters, etc. are also exported, too.

It's reasonable to say that if something warrants docs, it warrants an exported type, I think.

src/internal/types.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM. Just a nitpick.

src/internal/config.ts Outdated Show resolved Hide resolved
@jakovljevic-mladen
Copy link
Member Author

I think introducing and exporting the type is a small price to pay and we should make sure that all of the other types that are used for parameters, etc. are also exported, too.

I'll try to collect all other non-exported types and make a PR.

@jakovljevic-mladen
Copy link
Member Author

There is also one more thing that can be fixed (that is already fixed in the Angular docs) that I'll try to work on: basically enable links on the types. In the next image from Angular docs:

image

This is actually a link that opens a type, while in RxJS docs these are not links at all. That would be a handy addition to the docs.

@benlesh benlesh merged commit 02a9098 into ReactiveX:master Apr 29, 2021
@benlesh
Copy link
Member

benlesh commented Apr 29, 2021

Thank you, @jakovljevic-mladen!

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