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

Change interface to class in exported types #2157

Merged
merged 2 commits into from
Jun 30, 2022
Merged

Change interface to class in exported types #2157

merged 2 commits into from
Jun 30, 2022

Conversation

flappyBug
Copy link
Contributor

@flappyBug flappyBug commented Jun 28, 2022

Class constructor should be exported as class instead of interface.

Why does this matter? Interface is removed in runtime after compilation. But actually, types like Logger is accessible in pure js. For my use case, I'll need a runtime type as a dependency injection token, as well as a well-typed interface.

Another reason is class is inheritable, but if one wants to make a class extending interface, he/she should implement all the methods declared in interface.

Ref: https://stackoverflow.com/questions/14345485/whats-the-difference-between-declare-class-and-interface-in-typescript

@flappyBug
Copy link
Contributor Author

Can someone help to review this?

@wbt
Copy link
Contributor

wbt commented Jun 29, 2022

It seems OK to me, as those types are classes.
Note that at least from me and likely more generally you'll get faster, simpler reviews if you separate functional changes from formatting changes (e.g. removing spaces, adding line breaks, double to single quotes, etc.).

@flappyBug
Copy link
Contributor Author

Ah, you're right. For the style change, it's auto formatted by my IDE. I noticed that and didn't revert that because I saw editorconfig and prettier configured in the project. The formatting just follows those configs.

@flappyBug
Copy link
Contributor Author

And I think we'd better have style check in CI for better collaboration. But that's out of scope for this PR.

@wbt wbt merged commit 6217120 into winstonjs:master Jun 30, 2022
@wbt
Copy link
Contributor

wbt commented Jun 30, 2022

Out in 3.8.1.

This was referenced Aug 24, 2022
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.

2 participants