[lexical] Chore: Change alias from type to interface for EditorThemeClasses#8190
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
EditorThemeClassesEditorThemeClasses
|
A better way forward is to move configuration of anything not built-in to extension config. These don’t have the same namespace issues, are type safe, etc. and you don’t need declaration merging to get there. |
|
Although objects of any type can be passed to extensions, the argument of the |
|
You can get a reference to any extension from those methods with https://lexical.dev/docs/api/modules/lexical_extension#getextensiondependencyfromeditor |
|
I see, it would look something like this // App.tsx
type MyTheme = EditorThemeClasses & {
tooltip?: string
}
export const editorExtension = defineExtension<
ExtensionConfigBase,
'My App,
{theme: MyTheme},
unknown
>({
name: "[root]",
namespace: "My App",
dependencies: [
RichTextExtension,
configExtension(ReactExtension, { contentEditable: null }),
],
nodes: [TooltipNode],
theme: { tooltip: 'MyEditorTheme__tooltip' },
});
// TooltipNode.ts
createDOM(config: EditorConfig, editor: LexicalEditor): HTMLElement {
const element = document.createElement('span');
// Theme will also be available in the config argument.
// A cleaner option is to define a separate extension for a custom theme
// or define the node theming in the node extension itself.
const {output} = getExtensionDependencyFromEditor(editor, editorExtension);
addClassNamesToElement(element, output.theme.tooltip);
return element;
}Well, I agree that this is a cleaner way and probably the only correct one if the user needs to avoid namespace conflicts and support multiple config themes. However, in simpler cases, declaration merging seems more appealing way. At least because the theme object is already available in DOM methods. It is also simply a familiar and fairly common way of extending types for such purposes, and it is also recommended by some large projects, for example:
If you are concerned that this approach will become widespread, I can change the PR description so that users do not consider this method to be officially recommended |
|
No, that's not really at all how it would look, but there is an example https://lexical.dev/docs/extensions/migration#react-plug-in-without-ui if you choose the "After (all-in)" tab. |
etrepum
left a comment
There was a problem hiding this comment.
This change is fine, especially since the TypeScript compiler generally performs better with interfaces, but declaration merging and ambient types are not lexical best practices. I would not encourage the use case proposed in the PR description.
etrepum
left a comment
There was a problem hiding this comment.
On second thought the changes to the flow types should be reverted. Flow doesn't do declaration merging so there's no reason to make any changes there.
8a82ca1 to
8638a46
Compare
Description
This change allows you to extend the theme object type with indexable specific properties and take advantage of IDE and TypeScript hints
Test plan
Before
The
EditorThemeClassestype is based on an indexed signature:This means that if you are using TypeScript, you can declare a theme with any properties that differ from the standard ones, but TypeScript will not know about this and will cast others properties to the
anytypeAfter
You can still assign any properties to the
EditorThemeClassesobject, but using TypeScript, you can extend it with specific properties to target IDE hintsThe best practice is to define custom themes for nodes through the Extension API. See the example in the documentation: https://lexical.dev/docs/extensions/migration#react-plug-in-without-ui