-
Notifications
You must be signed in to change notification settings - Fork 177
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
Code: Added new types package for TS #12301
Conversation
What‘s the advantage of also moving things like the pattern types to this new package? |
Mostly to avoid circular references. The types package should be at the bottom of the dependency tree IMO and thus not depend on anything else, but it needs the patterns for various color properties, so this made sense to me. This can of course be reversed. |
I see. There’s definitely a lot to untangle with all the element types and such. Patterns don‘t have any other dependencies, so there should be no circular references in that instance. But let‘s look at it once this progresses further 👍 |
Plugin builds for f73c8dc are ready 🛎️!
|
Size Change: +23 B (0%) Total Size: 2.72 MB
ℹ️ View Unchanged
|
To iterate on my previous comment, the main things standing out:
There shouldn't be any circular dependencies that way. I can see the appeal of a catch-all types package at the bottom of the dependency tree though. So not sure what works best... |
locked: boolean; | ||
} | ||
|
||
export interface Element extends ElementBox { |
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.
Maybe it would be good to add a union type of all the elements as well for using in other packages? We're very often using element
as a prop without knowing which element type it is exactly. Maybe element
could be a union type and baseElement
(or sth similar) what it currently is?
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.
I see why you want that, but it's kind of not the right approach here. Rather, if you're looking through the array of elements for say TextElement
's, to manipulate all text elements, you would do so with a user-defined type predicate (we could easily create those along with each element type, but they're also trivial to define when needed):
function isTextElement(e: Element): e is TextElement {
return e.type === 'text';
}
const textElements = page.elements.filter(isTextElement);
The latter variable here now has TextElement[]
type. It can also be defined inline:
page.elements.filter((e: Element): e is TextElement => e.type === 'text');
That x is Foo
syntax looks weird, but it works, so 🤷.
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.
What if it's a function that actually accepts all types, for example:
function getLayerName(element) {
if (element.layerName) {
return element.layerName;
}
if (element.isBackground) {
return __('Background', 'web-stories');
}
return getDefinitionForType(element.type).getLayerText(element);
}
If we would assign just element: Element
, it would complain about isBackground
not existing (it exists on shapes and media only but not on Element
).
Thoughts?
I think it's a lot of work to do "types belong in the relevant package" organisation, but doable if we're diligent. Some of the utility functions inside the media package need access to element types (e.g. That means that the abstract catch-all element type would exist in the types package, the abstract media element type would exist in the media package, and the concrete element types (some extending element, others extending media element) would exist in the element library package. Sure, it would work, but it would also be a bit confusing in my opinion. Another option would be to move the I also notice that the element library package depends on a ton of other packages, so none of those can ever depend on any specific element type, which would seem to hamper us a bit. E.g. the animation package, which have methods referring to media elements (e.g. in I think the problem with this "types belong in their packages" approach is that we often combine both core functionality and broader utility functions inside a package - those broader utility functions often need types from other packages, but that creates a mess in dependencies. While having all the types in a single package perhaps isn't best-practice, it would make things a lot simpler. Would it make a difference if we turned these general types in the type package into more generic interfaces, of which actual elements would implement one or more? So we have Such a solution feels more "computer sciencey", but also more complex, as our actual elements are super concrete and such abstractions would be an almost useless exercise except to make the type system consistent. WDYT? |
# Conflicts: # packages/patterns/src/generatePatternStyles.ts
Those are good points, thanks for bringing them up. This shows that we still have a ways to go to improve our dependencies and avoid circular references.
The media package should not need access to element types.
Are there others utils in
I think we should just get rid of the
Since Maybe
Another implicit dependency that we should strive to avoid. In fact, it's the only such dependency on the text element type in the This could be solved by moving the file to const pasteTextContent = usePasteTextContent((content) => {
insertElement('text', { ...DEFAULT_PRESET, content });
});
That sounds like a sign to split up packages.
That is definitely true. And I think it would be OK to do that for now, but we should strive to fix the circular dependency issues so that we no longer have to do this. |
When the packages were being created, I remember it was often not clear what belonged to where, adding types seems to be a great opportunity to improve all that as well. Agreed that it would be ideal to have the types with the correspondent packages, it would widen the scope of this issue quite a lot though. Sounds like the simplest way would be using the shared types package as Morten currently has in this PR and then creating follow-up issues for moving the types where they belong together with improving the dependencies and the organization of the packages. |
Alright, let's get this merged to unblock a number of other issues (which would require a ✅ after I've fixed all pending issues), and then I'll create some followups to organize some of the other packages better. |
I feel it makes sense to "postpone" the moving of types to their relevant packages to when we convert the element library package. Yes, there'll be a lot of references to update, but I think it makes sense to do only then. In the meantime, we could make another ticket to reduce some of the references between packages? Specifically:
|
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.
LGTM as the initial version 🔨
(pending the missing metrics
)
Context
This adds a new TS type-only package.
Testing Instructions
Checklist
Type: XYZ
label to the PRFixes #12090