[Lexical][UI Gallery] Fix meta lexical website intern build errors #8
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
…ages and apis related autogenerated docs
|
Clone of the PR facebook/lexical#6300 |
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
WalkthroughThe changes introduce conditional logic in the Docusaurus configuration files based on the Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| const {packagesManager} = process.env.FB_INTERNAL | ||
| ? {} | ||
| : require('../../scripts/shared/packagesManager'); |
There was a problem hiding this comment.
I noticed that you're using environment variables to conditionally import packages and configure the application. This is a good practice for separating configuration from code, but it's important to ensure that these environment variables are correctly set in all environments where the application will be run. I recommend adding error handling to provide clear error messages if the required environment variables are not set. This will make it easier to diagnose and fix any issues related to environment configuration.
Chat with Korbit for more details or suggestions by mentioning @korbit-ai in your reply, and don't forget to give a 👍 or 👎 to help Korbit improve your reviews.
| const {packagesManager} = process.env.FB_INTERNAL | ||
| ? {} | ||
| : require('../../scripts/shared/packagesManager'); |
There was a problem hiding this comment.
The conditional import of packagesManager could be improved for better error handling and maintainability. Consider using a dynamic import or a try-catch block to handle cases where the import might fail, especially in the FB_INTERNAL environment. This would make the code more robust and prevent potential runtime errors.
Chat with Korbit for more details or suggestions by mentioning @korbit-ai in your reply, and don't forget to give a 👍 or 👎 to help Korbit improve your reviews.
| process.env.FB_INTERNAL | ||
| ? null | ||
| : [ | ||
| './plugins/package-docs', | ||
| /** @type {import('./plugins/package-docs').PackageDocsPluginOptions} */ | ||
| { | ||
| baseDir: path.resolve(__dirname, '..'), | ||
| editUrl: `${GITHUB_REPO_URL}/tree/main/packages/`, | ||
| packageFrontMatter: { | ||
| lexical: [ | ||
| 'sidebar_position: 1', | ||
| 'sidebar_label: lexical (core)', | ||
| ].join('\n'), | ||
| }, | ||
| targetDir: path.resolve(__dirname, 'docs/packages'), | ||
| }, | ||
| ], |
There was a problem hiding this comment.
The code is using process.env.FB_INTERNAL to conditionally include or exclude certain plugins and configurations based on whether the build is internal or not. However, it does not handle the case where FB_INTERNAL is undefined. This could lead to runtime errors if that environment variable is not set. To fix this, provide a default value using the nullish coalescing operator (??) to default undefined values to false. For example, change process.env.FB_INTERNAL ? null : [ ... ] to (process.env.FB_INTERNAL ?? false) ? null : [ ... ].
Chat with Korbit for more details or suggestions by mentioning @korbit-ai in your reply, and don't forget to give a 👍 or 👎 to help Korbit improve your reviews.
|
@coderabbitai review |
Actions performedReview triggered.
|
|
/review |
PR Reviewer Guide 🔍(Review updated until commit 56a7734)
|
github-actions
bot
added
Review effort [1-5]: 3
and removed
Review effort [1-5]: 2
labels
|
Persistent review updated to latest commit 56a7734 |
|
@coderabbitai full review |
Actions performedFull review triggered. |
codiumai-pr-agent-free
bot
added
Review effort [1-5]: 2
and removed
Review effort [1-5]: 3
labels
|
Persistent review updated to latest commit 56a7734 |
PR Code Suggestions ✨
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/lexical-website/docusaurus.config.js (6 hunks)
- packages/lexical-website/sidebars.js (1 hunks)
Additional comments not posted (2)
packages/lexical-website/sidebars.js (1)
56-70: Verify the environment variable setup forFB_INTERNAL.The conditional logic for the "Packages" section in the sidebar is based on the
FB_INTERNALenvironment variable. Ensure that this variable is set correctly in all environments where the application is deployed to avoid runtime issues.Run the following script to verify the usage of the
FB_INTERNALenvironment variable across the codebase:packages/lexical-website/docusaurus.config.js (1)
243-259: Ensure error handling for undefined environment variables.The
pluginsconfiguration should handle cases whereFB_INTERNALis undefined to prevent runtime errors.Consider using a default value with the nullish coalescing operator (
??) to handle undefined values:plugins: (process.env.FB_INTERNAL ?? false) ? null : [ './plugins/package-docs', /** @type {import('./plugins/package-docs').PackageDocsPluginOptions} */ { baseDir: path.resolve(__dirname, '..'), editUrl: `${GITHUB_REPO_URL}/tree/main/packages/`, packageFrontMatter: { lexical: [ 'sidebar_position: 1', 'sidebar_label: lexical (core)', ].join('\n'), }, targetDir: path.resolve(__dirname, 'docs/packages'), }, ],Likely invalid or redundant comment.
| process.env.FB_INTERNAL | ||
| ? { | ||
| href: 'https://lexical.dev/docs/api/', | ||
| label: 'API', | ||
| position: 'left', | ||
| } | ||
| : { | ||
| label: 'API', | ||
| position: 'left', | ||
| sidebarId: 'api', | ||
| type: 'docSidebar', | ||
| }, |
There was a problem hiding this comment.
Ensure error handling for undefined environment variables.
The API link configuration should handle cases where FB_INTERNAL is undefined to prevent runtime errors.
Consider using a default value with the nullish coalescing operator (??) to handle undefined values:
process.env.FB_INTERNAL ?? false
? {
href: 'https://lexical.dev/docs/api/',
label: 'API',
position: 'left',
}
: {
label: 'API',
position: 'left',
sidebarId: 'api',
type: 'docSidebar',
},Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| process.env.FB_INTERNAL | |
| ? { | |
| href: 'https://lexical.dev/docs/api/', | |
| label: 'API', | |
| position: 'left', | |
| } | |
| : { | |
| label: 'API', | |
| position: 'left', | |
| sidebarId: 'api', | |
| type: 'docSidebar', | |
| }, | |
| process.env.FB_INTERNAL ?? false | |
| ? { | |
| href: 'https://lexical.dev/docs/api/', | |
| label: 'API', | |
| position: 'left', | |
| } | |
| : { | |
| label: 'API', | |
| position: 'left', | |
| sidebarId: 'api', | |
| type: 'docSidebar', | |
| }, |
| entryPoints: process.env.FB_INTERNAL | ||
| ? [] | ||
| : packagesManager | ||
| .getPublicPackages() | ||
| .flatMap((pkg) => | ||
| pkg | ||
| .getExportedNpmModuleEntries() | ||
| .map((entry) => [ | ||
| path.relative( | ||
| __dirname, | ||
| pkg.resolve('src', entry.sourceFileName), | ||
| ), | ||
| ]), | ||
| ), |
There was a problem hiding this comment.
Ensure error handling for undefined environment variables.
The entryPoints configuration should handle cases where FB_INTERNAL is undefined to prevent runtime errors.
Consider using a default value with the nullish coalescing operator (??) to handle undefined values:
entryPoints: (process.env.FB_INTERNAL ?? false)
? []
: packagesManager
.getPublicPackages()
.flatMap((pkg) =>
pkg
.getExportedNpmModuleEntries()
.map((entry) => [
path.relative(
__dirname,
pkg.resolve('src', entry.sourceFileName),
),
]),
),Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| entryPoints: process.env.FB_INTERNAL | |
| ? [] | |
| : packagesManager | |
| .getPublicPackages() | |
| .flatMap((pkg) => | |
| pkg | |
| .getExportedNpmModuleEntries() | |
| .map((entry) => [ | |
| path.relative( | |
| __dirname, | |
| pkg.resolve('src', entry.sourceFileName), | |
| ), | |
| ]), | |
| ), | |
| entryPoints: (process.env.FB_INTERNAL ?? false) | |
| ? [] | |
| : packagesManager | |
| .getPublicPackages() | |
| .flatMap((pkg) => | |
| pkg | |
| .getExportedNpmModuleEntries() | |
| .map((entry) => [ | |
| path.relative( | |
| __dirname, | |
| pkg.resolve('src', entry.sourceFileName), | |
| ), | |
| ]), | |
| ), |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/lexical-website/sidebars.js (1)
56-70: Conditional rendering for "Packages" section looks good, but consider some improvements.The implementation successfully differentiates between internal and external builds, which aligns with the PR objectives. However, there are a few suggestions for improvement:
- The indentation of the new code block seems off. Consider adjusting it for better readability.
- Using
process.env.FB_INTERNALdirectly in the sidebar configuration might not be the most maintainable approach. Consider extracting this logic to a separate configuration file or utility function.- The external URL is hardcoded. It might be better to define it as a constant or configuration variable for easier maintenance.
Here's a suggested refactor to address these points:
const config = require('./config'); // ... (rest of the file) const sidebars = { // ... (other sidebar items) config.isInternalBuild() ? { href: config.EXTERNAL_PACKAGES_URL, label: 'Packages', type: 'link', } : { items: [ { dirName: 'packages', type: 'autogenerated', }, ], label: 'Packages', type: 'category', }, // ... (rest of the sidebar items) };This approach improves maintainability and readability while preserving the intended functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/lexical-website/docusaurus.config.js (6 hunks)
- packages/lexical-website/sidebars.js (1 hunks)
🧰 Additional context used
| const {packagesManager} = process.env.FB_INTERNAL | ||
| ? {} | ||
| : require('../../scripts/shared/packagesManager'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to use a constant for process.env.FB_INTERNAL
To improve readability and maintainability, consider defining a constant to store the value of process.env.FB_INTERNAL and reuse it throughout the file. This reduces redundancy and makes future updates easier.
Define the constant at the beginning of the file:
+const IS_FB_INTERNAL = Boolean(process.env.FB_INTERNAL);Then update the conditional statements:
-const {packagesManager} = process.env.FB_INTERNAL
+const {packagesManager} = IS_FB_INTERNAL
? {}
: require('../../scripts/shared/packagesManager');- entryPoints: process.env.FB_INTERNAL
+ entryPoints: IS_FB_INTERNAL
? []
: packagesManager
.getPublicPackages()
.flatMap((pkg) =>
pkg
.getExportedNpmModuleEntries()
.map((entry) => [
path.relative(
__dirname,
pkg.resolve('src', entry.sourceFileName),
),
]),
),- process.env.FB_INTERNAL
+ IS_FB_INTERNAL
? null
: [
'./plugins/package-docs',
/** @type {import('./plugins/package-docs').PackageDocsPluginOptions} */
{
baseDir: path.resolve(__dirname, '..'),
editUrl: `${GITHUB_REPO_URL}/tree/main/packages/`,
packageFrontMatter: {
lexical: [
'sidebar_position: 1',
'sidebar_label: lexical (core)',
].join('\n'),
},
targetDir: path.resolve(__dirname, 'docs/packages'),
},
],- process.env.FB_INTERNAL
+ IS_FB_INTERNAL
? {
href: 'https://lexical.dev/docs/api/',
label: 'API',
position: 'left',
}
: {
label: 'API',
position: 'left',
sidebarId: 'api',
type: 'docSidebar',
},Also applies to: 175-188, 243-259, 390-401
User description
WHAT
Fix meta lexical website intern build errors
WHY
TEST
tested locally, everything works as is for Extern Lexical Website
Description by Korbit AI
Note
This feature is in early access. You can enable or disable it in the Korbit Console.
What change is being made?
Fix meta lexical website internal build errors by conditionally requiring
packagesManagerand adjusting configurations based on theFB_INTERNALenvironment variable.Why are these changes being made?
These changes address build errors encountered in the internal Meta environment by ensuring that certain configurations and plugins are only applied when not in the internal environment. This approach maintains compatibility with both internal and external builds, preventing unnecessary failures.
PR Type
enhancement
Description
docusaurus.config.jsto handle internal Meta environment builds by checking theFB_INTERNALenvironment variable.entryPointsand plugins configuration to exclude certain elements when in the internal environment.sidebars.jsto use external links for 'Packages' when in the internal environment.Changes walkthrough 📝
docusaurus.config.js
Conditional configuration for internal Meta environmentpackages/lexical-website/docusaurus.config.js
packagesManagerbased onFB_INTERNALenvironmentvariable.
entryPointsand plugins configuration for internal builds.FB_INTERNALis true.sidebars.js
Adjust sidebar configuration for internal buildspackages/lexical-website/sidebars.js
FB_INTERNALenvironmentvariable.
Summary by CodeRabbit
New Features
Bug Fixes
Chores