Skip to content

Support moduleResolution bundler#5716

Closed
2wheeh wants to merge 4 commits intofacebook:mainfrom
2wheeh:add-entry-for-types
Closed

Support moduleResolution bundler#5716
2wheeh wants to merge 4 commits intofacebook:mainfrom
2wheeh:add-entry-for-types

Conversation

@2wheeh
Copy link
Copy Markdown
Contributor

@2wheeh 2wheeh commented Mar 15, 2024 •
edited
Loading

fixes #5710
fixes #5117
fixes #4160

This will support correct type inference with moduleResolution: bundler option of tsconfig, which is default on creat next app, create vite.

Currently, when importing lexcial or @lexical/... with moduleResolution: bundler, .d.ts files resolves '.' to the main entry on package.json.
So if some types are imported with '.', this ends up being inferred as any.

For example, ElementNode here is imported from '.'.

image 
import { LexicalNode } from 'lexical'
const node = {} as LexicalNode;

node.getParent() // this should be inferred as `ElementNode | null`

image

This should have inferred like this. LexicalNode is not imported from '.' so it works:

image

With adding entry for types, it can infer correct type with moduleResolution: bundler.

image

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 15, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 18, 2024 6:09am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 18, 2024 6:09am

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 15, 2024
@2wheeh 2wheeh force-pushed the add-entry-for-types branch from e5e3a78 to 8362ad6 Compare March 15, 2024 08:10
@2wheeh
Copy link
Copy Markdown
Contributor Author

2wheeh commented Mar 15, 2024 •
edited
Loading

I tried to convert lexical's moduleResolution to bundler(392322a) but somehow it fails to deploy on Vercel.
So i just reverted it. I believe now is enough to make possible to use bundler in the project importing lexical.

Edit:
To convert moduleResolution to bundler:

@2wheeh 2wheeh force-pushed the add-entry-for-types branch from fe2bbac to cc37aba Compare March 15, 2024 09:01
@2wheeh 2wheeh changed the title Add entry for types Converted moduleResolution to bundler on tsconfig Mar 15, 2024
@2wheeh 2wheeh changed the title Converted moduleResolution to bundler on tsconfig Converted moduleResolution to bundler Mar 15, 2024
@2wheeh 2wheeh changed the title Converted moduleResolution to bundler Convert moduleResolution to bundler Mar 15, 2024
@2wheeh 2wheeh force-pushed the add-entry-for-types branch from 29af025 to 0f59ce4 Compare March 15, 2024 15:10
@2wheeh
Copy link
Copy Markdown
Contributor Author

2wheeh commented Mar 15, 2024 •
edited
Loading

Now, there aren't any tsc errors with either node or bundler

@2wheeh 2wheeh changed the title Convert moduleResolution to bundler Support moduleResolution bundler Mar 15, 2024
@2wheeh 2wheeh force-pushed the add-entry-for-types branch from b0a6902 to 983a50b Compare March 18, 2024 02:24
@2wheeh 2wheeh force-pushed the add-entry-for-types branch from cd53d07 to 2e67d5f Compare March 18, 2024 02:35
@2wheeh 2wheeh marked this pull request as draft March 18, 2024 02:41
@2wheeh 2wheeh force-pushed the add-entry-for-types branch from 2e67d5f to e373bc8 Compare March 18, 2024 05:16
@2wheeh 2wheeh force-pushed the add-entry-for-types branch from e373bc8 to 9071b9d Compare March 18, 2024 06:06
@2wheeh 2wheeh marked this pull request as ready for review March 18, 2024 06:10
@2wheeh
Copy link
Copy Markdown
Contributor Author

2wheeh commented Mar 18, 2024 •
edited
Loading

rebased and resolved new errors. I guess It's ready unless failing e2e-collab-windows (18.18.0, firefox) is due to this change.

e2e-collab-windows (18.18.0, firefox) keeps failing by being timed out ...

acywatson
acywatson reviewed Mar 18, 2024
View reviewed changes
packages/lexical-playground/package.json
"react-dom": "^18.2.0",
"react-error-boundary": "^3.1.4",
"y-websocket": ">=1.3.x",
"y-websocket": "^1.5.4",
Copy link
Copy Markdown
Contributor

@acywatson acywatson Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What’s this about?

Copy link
Copy Markdown
Contributor Author

@2wheeh 2wheeh Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with bundler, importing y-websocket throws an error
Pasted Graphic 3

y-websocket maintainer’s comment for fixing this: update to ^1.5.0
yjs/y-websocket#133 (comment)

@acywatson
Copy link
Copy Markdown
Contributor

acywatson commented Mar 18, 2024

What do you expect this to break (if anything) and what’s the upgrade path?

@2wheeh
Copy link
Copy Markdown
Contributor Author

2wheeh commented Mar 19, 2024

@acywatson

what’s the upgrade path?

With bundler,

// jest.config.js

'^@lexical/react/src/(.*)$': '<rootDir>/packages/lexical-react/src/$1',

this fails to resolve the paths somehow, leading unit tests to fail.

And also just aligning the path with the way we do in tsconfig path alias seems more easy to maintain. Different imports for same path were easily allowed. There are some more in unit test codes.

// i.e. both are used in unit tests

import {useLexicalComposerContext} from '@lexical/react/LexicalComposerContext';
import {useLexicalComposerContext} from '@lexical/react/src/LexicalComposerContext';

So I updated path matchers in jest config for unifying imports in unit tests and make it work with bundler.

What do you expect this to break (if anything)

I think it's fine with both node and bundler now. but not sure for the future for the other one since we develop with one. maybe we need a strategy to support both appropriately. (new test on CI ? or just supporting bundler covers both ?)

or

In certain apps using lexical with bundler, if they have put @ts-expect-error to address type inference failures, it must be removed with a lexical version update.

@2wheeh 2wheeh mentioned this pull request Mar 28, 2024
Merged
@2wheeh
Copy link
Copy Markdown
Contributor Author

2wheeh commented Mar 29, 2024

Close this since it is incorporating to #5774 .

@2wheeh 2wheeh closed this Mar 29, 2024
@2wheeh 2wheeh deleted the add-entry-for-types branch March 29, 2024 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@acywatson acywatson acywatson left review comments

@zurfyx zurfyx Awaiting requested review from zurfyx zurfyx is a code owner

@fantactuka fantactuka Awaiting requested review from fantactuka fantactuka is a code owner

@Fetz Fetz Awaiting requested review from Fetz

@ivailop7 ivailop7 Awaiting requested review from ivailop7 ivailop7 is a code owner

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

3 participants

@2wheeh @acywatson @facebook-github-bot