Refactored constants, interfaces and types to their respective files#48667
Conversation
This cleans up the code a lot, cause quite a few interfaces don't even need to be exported from this types file. It's also more consistent with a lot of other packages where types and interfaces already have their own file. Ran the build without any errors, but please take a second look to make sure I didn't miss anything. :)
dmsnell
left a comment
There was a problem hiding this comment.
I'm going to let @adamziel speak to this. Thanks for the contribution @tomdevisser. Personally I don't find this change clearer; in some ways I find it to be more indirect, which of course has less of an impact with TS code than JS code, but still some of the moves seem unnecessary vs. being colocated with the code using the types.
| Success = 'SUCCESS', | ||
| } | ||
|
|
||
| export const EMPTY_ARRAY = []; |
There was a problem hiding this comment.
in general it's probably best not to abstract these things. we'll end up using more code and running more code to get what we want, which is nothing more than an empty array. plus then we have to deal with the consequences of coupling modules together that may interfere.
There was a problem hiding this comment.
Fair point. Just thought it was a new convention we were working towards and I came across these, so thought it would be a nice quick refactor to make things more consistent. If more devs see it as a step in the wrong direction, we can close this one.
There was a problem hiding this comment.
EMPTY_ARRAY and EMPTY_OBJECT and the like are special-cases regardless, because those are incidental details covering over the fact that all literal objects in JS have unique references, so things that want shallow-compare have to use the same one in comparison. within a module these are essentially free, but once we start building that coupling between modules we start pulling in overhead and concerns that really don't belong there for such a tiny optimization.
|
Thanks for the contribution @tomdevisser! Personally, I prefer to have types colocated with the code like @dmsnell mentioned. Keeping the two tightly coupled parts in the same place saves jumping between the implementation and the type declarations. Would you mind saying a little bit more about your rationale? |
|
@adamziel No worries. Like I said I just spotted it for a a growing list of other components so I thought it would be more consistent, and consistency only works if done 100% imo, even if when zooming in on a single component/folder it's not super helpful at first sight. If you open a folder and come to expect constants and types to have their own files, but there are sometimes still constants and types declared somewhere else, this may lead to confusion. I understand both arguments given here by you both, but they both consider this folder on it's own, where I tried looking at the bigger picture while refactoring. However if my opinion on that is in the minority, I don't mind closing the PR. :) |
|
@tomdevisser I hear you! I don't think there's a standard way of placing types in Gutenberg. There are instances of |
What and how?
Refactored constants, interfaces and types to their respective files
Why?
This cleans up the code a lot, cause quite a few interfaces don't even need to be exported from this types file.
It's also more consistent with a lot of other packages where types and interfaces already have their own file.
Testing
Ran the build without any errors, but please take a second look to make sure I didn't miss anything. :) Not sure how to actually test it manually.
One note:
I tried moving
QuerySelectResponseto the types file as well, but my linter is giving an error when I import it for not being used. When I don't import it, I get a linter error for the return type here:So for now I have kept a copy of this interface in the file (
use-query-select.ts) just to keep the linter satisfied. If there's a way around this, or it's safe to silence the linter in the JSDoc, let me know.