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.

Sign up for GitHub

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

[Lexical][UI Gallery] Fix meta lexical website intern build errors #8

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[Lexical][UI Gallery] Fix meta lexical website intern build errors #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02f4797
[Lexical][Meta] Fix errors in intern meta static docs related to pack…
Sahejkm Jun 12, 2024
56a7734
[skip ci]
korbit-ai[bot] Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 48 additions & 32 deletions packages/lexical-website/docusaurus.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
const {github: lightCodeTheme, dracula: darkCodeTheme} =
require('prism-react-renderer').themes;
const slugifyPlugin = require('./src/plugins/lexical-remark-slugify-anchors');
const {packagesManager} = require('../../scripts/shared/packagesManager');
const {packagesManager} = process.env.FB_INTERNAL
? {}
: require('../../scripts/shared/packagesManager');
Comment on lines +16 to +18
Copy link
Author

Choose a reason for hiding this comment

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

category Systems and Environment

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.

Comment on lines +16 to +18
Copy link
Author

Choose a reason for hiding this comment

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

category Functionality

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.

Comment on lines +16 to +18
Copy link

Choose a reason for hiding this comment

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

🛠️ 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

const path = require('node:path');

const TITLE = 'Lexical';
Expand Down Expand Up @@ -170,15 +172,20 @@ const sidebarItemsGenerator = async ({
/** @type {Partial<import('docusaurus-plugin-typedoc/dist/types').PluginOptions>} */
const docusaurusPluginTypedocConfig = {
...sourceLinkOptions(),
entryPoints: packagesManager
.getPublicPackages()
.flatMap((pkg) =>
pkg
.getExportedNpmModuleEntries()
.map((entry) => [
path.relative(__dirname, pkg.resolve('src', entry.sourceFileName)),
]),
),
entryPoints: process.env.FB_INTERNAL
? []
: packagesManager
.getPublicPackages()
.flatMap((pkg) =>
pkg
.getExportedNpmModuleEntries()
.map((entry) => [
path.relative(
__dirname,
pkg.resolve('src', entry.sourceFileName),
),
]),
),
Comment on lines +175 to +188
Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
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),
),
]),
),

excludeInternal: true,
plugin: [
'./src/plugins/lexical-typedoc-plugin-no-inherit',
Expand Down Expand Up @@ -233,21 +240,23 @@ const config = {
onBrokenMarkdownLinks: 'throw',
organizationName: 'facebook',
plugins: [
[
'./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
? 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'),
},
],
Comment on lines +243 to +259
Copy link
Author

Choose a reason for hiding this comment

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

category Error Handling

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.

'./plugins/webpack-buffer',
['docusaurus-plugin-typedoc', docusaurusPluginTypedocConfig],
async function tailwindcss() {
Expand All @@ -260,7 +269,7 @@ const config = {
name: 'docusaurus-tailwindcss',
};
},
],
].filter((plugin) => plugin != null),

presets: [
[
Expand All @@ -274,6 +283,7 @@ const config = {
docs: {
beforeDefaultRemarkPlugins: [slugifyPlugin],
editUrl: `${GITHUB_REPO_URL}/tree/main/packages/lexical-website/`,
exclude: ['/error'],
path: 'docs',
sidebarItemsGenerator,
sidebarPath: require.resolve('./sidebars.js'),
Expand Down Expand Up @@ -377,12 +387,18 @@ const config = {
sidebarId: 'docs',
type: 'docSidebar',
},
{
label: 'API',
position: 'left',
sidebarId: 'api',
type: 'docSidebar',
},
process.env.FB_INTERNAL
? {
href: 'https://lexical.dev/docs/api/',
label: 'API',
position: 'left',
}
: {
label: 'API',
position: 'left',
sidebarId: 'api',
type: 'docSidebar',
},
Comment on lines +390 to +401
Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
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',
},


{label: 'Community', position: 'left', to: '/community'},
{
Expand Down
24 changes: 15 additions & 9 deletions packages/lexical-website/sidebars.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,16 +53,22 @@ const sidebars = {
label: 'Concepts',
type: 'category',
},
{
items: [
{
dirName: 'packages',
type: 'autogenerated',
process.env.FB_INTERNAL
? {
href: 'https://lexical.dev/docs/packages/lexical',
label: 'Packages',
type: 'link',
}
: {
items: [
{
dirName: 'packages',
type: 'autogenerated',
},
],
label: 'Packages',
type: 'category',
},
],
label: 'Packages',
type: 'category',
},
{
items: [{dirName: 'react', type: 'autogenerated'}],
label: 'React',
Expand Down