-
Notifications
You must be signed in to change notification settings - Fork 10
feat: upgrade NX and migrate builds to esbuild #1185
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
88f354b to
3cf8690
Compare
3cf8690 to
ef9ef69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 118 out of 135 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
sdk/nextjs/src/common/actions/setDebugUser.ts:1
- Using bracket notation for environment variables (
process.env['NODE_ENV']) is unnecessary. The dot notationprocess.env.NODE_ENVis more idiomatic and equally safe in TypeScript/Node.js environments.
sdk/nextjs/src/common/actions/clearDebugUser.ts:1 - Using bracket notation for environment variables (
process.env['NODE_ENV']) is unnecessary. The dot notationprocess.env.NODE_ENVis more idiomatic and equally safe in TypeScript/Node.js environments.
e2e/nextjs/app-router/tests/app-router.spec.ts:1 - The timeout was increased from 10000 to 30000ms. Consider adding a comment explaining why such a long timeout is necessary for this navigation test, as this could mask performance issues.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 120 out of 137 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth copying these changes elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its basically just formatting, so probably not
| PublicEnvironment, | ||
| PublicFeature, | ||
| PublicProject, | ||
| PublicTarget, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused import changes? or were these missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 120 out of 137 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Configure externals - use array format which is simpler and more reliable | ||
| if (options.external && Array.isArray(options.external)) { | ||
| // Convert to object format for webpack externals | ||
| const externalsObj = {} | ||
| options.external.forEach((pkg) => { | ||
| externalsObj[pkg] = `commonjs ${pkg}` | ||
| }) | ||
| const originalExternals = config.externals | ||
| if (originalExternals) { | ||
| if (Array.isArray(originalExternals)) { | ||
| config.externals = [...originalExternals, externalsObj] | ||
| } else if (typeof originalExternals === 'object') { | ||
| config.externals = { ...originalExternals, ...externalsObj } | ||
| } else { | ||
| config.externals = [originalExternals, externalsObj] | ||
| } | ||
| } else { | ||
| config.externals = externalsObj | ||
| } | ||
| } |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The externals configuration logic is complex and handles multiple array/object cases. This same pattern appears in multiple webpack configs. Consider extracting this into a shared utility function to reduce code duplication and improve maintainability across dev-apps/nodejs-local, dev-apps/openfeature-nodejs, and dev-apps/nodejs-cloud webpack configs.
| "external": [ | ||
| "shared-types", | ||
| "shared-bucketing-as", | ||
| "shared-bucketing-test-data", | ||
| "js-cloud-server-sdk", | ||
| "js" | ||
| ] | ||
| }, | ||
| "dependsOn": [ | ||
| { | ||
| "target": "build", | ||
| "dependencies": true | ||
| "@devcycle/types", | ||
| "@devcycle/bucketing-assembly-script", | ||
| "@devcycle/js-cloud-server-sdk", | ||
| "@devcycle/js-client-sdk", | ||
| "@openfeature/*", | ||
| "cross-fetch", | ||
| "eventsource", | ||
| "fetch-retry" |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SDK build has been migrated to esbuild with bundle: true and a comprehensive list of external packages. However, cross-fetch, eventsource, and fetch-retry are marked as external, which means they will be runtime dependencies. Verify that these packages are properly included in the package.json dependencies and not just devDependencies, as the bundled SDK will require them at runtime.
| "react": ">=16.8.0" | ||
| }, | ||
| "types": "./index.cjs.d.ts" | ||
| "types": "./index.d.ts" |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The types field has been changed from "./index.cjs.d.ts" to "./index.d.ts". Verify that the build output actually generates index.d.ts files at the package root, and not index.cjs.d.ts files. If esbuild generates the types with a different naming convention, this change may break TypeScript resolution for package consumers.
| "@devcycle/nodejs-server-sdk": "1.55.6" | ||
| }, | ||
| "main": "src/index.js", | ||
| "main": "index.js", |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main field has been changed from "src/index.js" to "index.js". This indicates that the build output structure has changed. Verify that the esbuild configuration actually outputs files to the package root (index.js) rather than maintaining the src/ directory structure. If the build outputs to a different location, package consumers will not be able to import this package.
| // Increase timeout for navigation in CI | ||
| await expect(page.getByText('Navigated Server Component')).toBeVisible({ | ||
| timeout: 10000, | ||
| timeout: 30000, // Increased timeout for CI environment variability |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeout has been increased from 10000ms to 30000ms with a comment stating "Increased timeout for CI environment variability". While this may address flaky tests in CI, such a large timeout increase (3x) could mask underlying performance issues or race conditions. Consider investigating the root cause of the timeout issues rather than simply increasing the timeout value.
| "packageManager": "yarn@4.9.2", | ||
| "scripts": { | ||
| "lint": "nx run-many --parallel --target lint --all", | ||
| "lint": "nx run-many --parallel --target check-types --all && nx run-many --parallel --target lint --all", |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lint script now runs check-types before lint. This means type checking will run twice - once explicitly via check-types and again as part of the lint process if ESLint has TypeScript rules enabled. This could slow down the CI pipeline. Consider whether both are necessary or if type checking should only be done once.
| { | ||
| "extends": "./tsconfig.lib.json", | ||
| "compilerOptions": { | ||
| "paths": {} | ||
| } | ||
| } |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new tsconfig.build.json file has been added that extends tsconfig.lib.json and clears the paths. However, the project.json now references this new file instead of tsconfig.lib.json. Verify that this doesn't break any workspace path aliases that might be needed during the build process. The empty paths object may prevent proper resolution of internal workspace dependencies.
| // Fix __dirname replacement for bucketing-assembly-script to use absolute path | ||
| // This ensures path.resolve(__dirname) works correctly | ||
| if (!config.plugins) { | ||
| config.plugins = [] | ||
| } | ||
| const webpack = require('webpack') | ||
| config.plugins.push( | ||
| new webpack.DefinePlugin({ | ||
| __dirname: JSON.stringify( | ||
| path.resolve( | ||
| __dirname, | ||
| '../../lib/shared/bucketing-assembly-script', | ||
| ), | ||
| ), | ||
| }), | ||
| ) |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manual path resolution using __dirname and path.resolve may be fragile. The webpack DefinePlugin is being used to override __dirname globally, which could have unintended side effects on other parts of the bundled code. A more targeted approach, such as using webpack's resolve.alias for the specific bucketing-assembly-script module, would be safer.
Summary
@nx/js:tscbuilds to@nx/esbuild:esbuilddev-apps/next, thee2e/nextjsapps work for testing + e2e testsWhy
NX 22 removed the experimental
externaloption from@nx/js:tscthat was used to bundle internal workspace packages. These packages are not published to npm and must be bundled into the SDK dist. Additionally, NX 22 introduced breaking changes requiring updates to webpack configs, Jest configs, and project.json files.Changes
tsctoesbuildfor faster buildsproject.jsonfiles with new executor configurationsTesting