Skip to content

Conversation

@jonathannorris
Copy link
Member

@jonathannorris jonathannorris commented Nov 25, 2025

Summary

  • Upgrade NX from 16.10.0 to 22.1.1
  • Migrate all @nx/js:tsc builds to @nx/esbuild:esbuild
  • Update webpack configs and project configurations for NX 22 compatibility
  • Update E2E workflow to pre-build shared libraries
  • Removed Next.js dev-apps/next, the e2e/nextjs apps work for testing + e2e tests

Why
NX 22 removed the experimental external option from @nx/js:tsc that 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

  • Build system: Migrated all SDKs/libs from tsc to esbuild for faster builds
  • nodejs, js-cloud-server: Now use esbuild to bundle internal dependencies
  • Webpack configs: Updated dev-apps (js, openfeature-web, cloudflare-worker, nodejs-local, openfeature-nodejs) and e2e tests for NX 22 compatibility
  • Project configs: Updated all project.json files with new executor configurations
  • Jest configs: Updated to use NX 22 patterns across all packages
  • E2E workflow: Added shared library build step before running affected E2E tests
  • Fixes: Updated e2e test ports, fixed type exports, resolved rollup build issues for react-native-expo, preserved Next.js 'use client' directives

Testing

  • All SDK tests pass (nodejs: 126, js-cloud-server: 71, bucketing: 222)
  • Verified against local example apps
  • E2E tests updated and verified

@vercel
Copy link

vercel bot commented Nov 25, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
js-sdks-web-elements Ready Ready Preview, Comment Jan 9, 2026 4:28pm
js-sdks-with-provider Ready Ready Preview, Comment Jan 9, 2026 4:28pm
1 Skipped Deployment
Project Deployment Review Updated (UTC)
js-sdks-next-js-page-router Ignored Ignored Jan 9, 2026 4:28pm

Copy link
Contributor

Copilot AI left a 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 notation process.env.NODE_ENV is 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 notation process.env.NODE_ENV is 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.

Copy link
Contributor

Copilot AI left a 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.

Copy link
Member

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?

Copy link
Member Author

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,
Copy link
Contributor

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?

Copy link
Contributor

Copilot AI left a 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.

Comment on lines +44 to +63
// 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
}
}
Copy link

Copilot AI Jan 9, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines 58 to +66
"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"
Copy link

Copilot AI Jan 9, 2026

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.

Copilot uses AI. Check for mistakes.
"react": ">=16.8.0"
},
"types": "./index.cjs.d.ts"
"types": "./index.d.ts"
Copy link

Copilot AI Jan 9, 2026

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.

Copilot uses AI. Check for mistakes.
"@devcycle/nodejs-server-sdk": "1.55.6"
},
"main": "src/index.js",
"main": "index.js",
Copy link

Copilot AI Jan 9, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines 49 to +50
// Increase timeout for navigation in CI
await expect(page.getByText('Navigated Server Component')).toBeVisible({
timeout: 10000,
timeout: 30000, // Increased timeout for CI environment variability
Copy link

Copilot AI Jan 9, 2026

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.

Copilot uses AI. Check for mistakes.
"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",
Copy link

Copilot AI Jan 9, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +6
{
"extends": "./tsconfig.lib.json",
"compilerOptions": {
"paths": {}
}
}
Copy link

Copilot AI Jan 9, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines +10 to +25
// 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',
),
),
}),
)
Copy link

Copilot AI Jan 9, 2026

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.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants