Skip to content
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

Bundle Lexical DevTools with Vite #2415

Merged
merged 6 commits into from
Jun 16, 2022

Conversation

noi5e
Copy link
Contributor

@noi5e noi5e commented Jun 15, 2022

This does everything that PR #2388 did, it just creates a browser extension scaffold with Vite instead of Create React App/webpack.

Lots of file/folder structural changes, but not any functional difference for the user. Even the installation instructions in README.md are the same.

I'm still figuring out how to do the Vite config. Right now the assets are split into two folders:

  1. src - React components
  2. public - I made this folder as a workaround, in order to get Vite to include manifest.json (and everything it references) within the build. Following Vite docs.

It's pretty crucial that the browser extension build includes a manifest.json. It seems like Vite doesn't do this out of the box.

An outside repo makes a custom Vite plugin that will generate a manifest.json, and it looks relatively easy to replicate. I haven't done this yet, will save it for another PR.


Developer Tools Web Extension Planning Issue: #2127

@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 Jun 15, 2022
@vercel
Copy link

vercel bot commented Jun 15, 2022

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

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Jun 15, 2022 at 9:25PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Jun 15, 2022 at 9:25PM (UTC)

@noi5e
Copy link
Contributor Author

noi5e commented Jun 15, 2022

I think this should be a simple fix, but I don't have the access to resolve the conflict:
Screen Shot 2022-06-14 at 7 12 03 PM

@thegreatercurve
Copy link
Contributor

@noi5e You shouldn't need any kind of special access to resolve the merge conflict. Update main on your fork, and then rebase your branch on top of that before merging into this repo.

Copy link
Contributor

@thegreatercurve thegreatercurve left a comment

Choose a reason for hiding this comment

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

Set up looks fine, but how com we've got rid of TypeScript? That should work out of the box for Vite. See the react-ts example here: https://vitejs.dev/guide/#trying-vite-online

packages/lexical-devtools/src/main.jsx Outdated Show resolved Hide resolved
packages/lexical-devtools/src/main.jsx Outdated Show resolved Hide resolved
packages/lexical-devtools/src/main.jsx Outdated Show resolved Hide resolved
@noi5e
Copy link
Contributor Author

noi5e commented Jun 15, 2022

@thegreatercurve Thanks for the assist, and helping this to pass the CI! Looks like the last few reviews you mentioned were covered by your commit as well.

This might be ready to merge, although I do have a slight question mark about deleting tsconfig.json and if this is now adequately covered by TypeScript from the root config (and not, let's say, from a global install in my environment?). I think it is? But not sure. Dropping a note in chat about this.

@noi5e noi5e requested a review from thegreatercurve June 15, 2022 22:21
@thegreatercurve
Copy link
Contributor

@thegreatercurve Thanks for the assist, and helping this to pass the CI! Looks like the last few reviews you mentioned were covered by your commit as well.

This might be ready to merge, although I do have a slight question mark about deleting tsconfig.json and if this is now adequately covered by TypeScript from the root config (and not, let's say, from a global install in my environment?). I think it is? But not sure. Dropping a note in chat about this.

@noi5e your folder is included in the includes of the base tsconfig, so if running tsc in the root folder doesn't cause any errors, you should be all good!

@thegreatercurve thegreatercurve merged commit ce201e9 into facebook:main Jun 16, 2022
@noi5e noi5e deleted the bundle-with-vite branch June 16, 2022 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

3 participants