[lexical-react] Breaking change: Deprecate public default exports #6088
Conversation
facebook-github-bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -7,6 +7,8 @@ | |||
| * @flow strict | |||
| */ | |||
|
|
|||
| declare export function MLCClickableLinkPlugin({ | |||
| declare export function ClickableLinkPlugin({ | |||
There was a problem hiding this comment.
This seemed wrong, since no such name was present. Not sure what was going on here?
There was a problem hiding this comment.
Let's keep it as is and mark deprecated
etrepum
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz and
ivailop7
as code owners
There was a problem hiding this comment.
This change (and the lint rule) really really good in a sense that it allows to unify approach to exports.
However breaking changes here seems to be unnecessary... This will be especially painful for larger codebases.
Better approach would be to do both named + default export while marking default as deprecated, announce change, let people migrate at their pace, deprecate defaults.
But let's hear other opinions too!
P.S.: Merge to Meta's WWW may be painful with this
|
This PR does that, default exports are still there, but it's breaking in the sense that working with the newer docs/examples won't be compatible with older lexical |
|
Also technically in cjs this is a breaking change if the bundler doesn't do the right thing with exports named default with these rollup settings |
| @@ -7,6 +7,8 @@ | |||
| * @flow strict | |||
| */ | |||
|
|
|||
| declare export function MLCClickableLinkPlugin({ | |||
| declare export function ClickableLinkPlugin({ | |||
There was a problem hiding this comment.
Let's keep it as is and mark deprecated
yep, I see now, by bad. Still asleep here :P |
|
@StyleT we could keep that flow type as-is, but it references a symbol that didn't exist in the first place? Would probably be better to remove it if using a name that exists isn't the proper fix |
yep, you're right. It's just a flow file. Good to be merged. Let's allow others at least a day to look at it |
Description
There are a handful of reasons to avoid default exports described briefly in #6079. A compelling reason not to use them is in our case is that they are rarely and inconsistently used in monorepo entrypoints, but there are other more technical reasons that they are not ideal (they generate confusing API docs for example)
@deprecatedcommentsimport * as React from 'react'is forgotten. I wonder why this isn't the default preset yet, given that it's been available for a few years.@flow strictinto the boilerplate template header correctlyThis is isolated to the
@lexical/rectpackage and affects the following modules:@lexical/react/LexicalClickableLinkPlugin(now exports nameClickableLinkPlugin)@lexical/react/LexicalErrorBoundary(now exports nameLexicalErrorBoundary)@lexical/react/LexicalTableOfContents->@lexical/react/LexicalTableOfContentsPlugin(which exports nameTableOfContentsPlugin) - the old module still exists with only a default export@lexical/react/useLexicalEditable(now exports nameuseLexicalEditable)@lexical/react/useLexicalSubscription(now exports nameuseLexicalSubscription)Closes: #6079
Test plan
module.exports = function defaultExport() {}; module.exports.defaultExport = module.exports;which is pretty much the only thing that would have the same cjs semantics. It could also of course be fixed with a codemod fairly easily as there are only five renames here.