feat(opensourceframework): major modernization and package updates#22
Closed
riceharvest wants to merge 2 commits intomainfrom
Closed
feat(opensourceframework): major modernization and package updates#22riceharvest wants to merge 2 commits intomainfrom
riceharvest wants to merge 2 commits intomainfrom
Conversation
- Upgraded multiple packages to modern standards (Next.js, Next-auth, PWA, SEO). - Added new utility packages: critters, next-circuit-breaker, next-csrf, next-images, next-json-ld. - Integrated Changesets for versioning. - Updated CI/CD workflows and linting configurations. - Fixed numerous linting and type-checking issues across the monorepo.
Review Summary by QodoMajor framework modernization with testing migrations, new package implementations, and infrastructure standardization
WalkthroughsDescription• **Major Testing Framework Migrations**: Migrated multiple packages from Jest/Vitest to native Node.js testing (node:test), and from Jest to Vitest across react-virtualized components • **New Package Implementations**: Added comprehensive implementations for next-compose-plugins, next-optimized-images with webpack loaders, SVG sprite handling, and LQIP support • **React Query Auth Example**: Created complete Vite-based example with MSW mocking, authentication flows, API client, and token management utilities • **Type System Improvements**: Enhanced type exports in next-session, relaxed crypto type constraints in next-iron-session, updated cookie type imports in next-csrf • **Build Configuration Updates**: Simplified and standardized tsup configurations across packages, added new build configs for next-auth, react-query-auth, and next-compose-plugins • **Placeholder Implementations**: Replaced comprehensive test suites and implementations with placeholders for next-images and next-csrf pending original package forks • **Bug Fixes**: Fixed middleware execution chain in next-connect, improved time parsing in next-session, removed TypeScript ignore directives in next-pwa • **Infrastructure & Tooling**: Added Prettier configurations across packages, updated ESLint for testing frameworks, created CI/CD synchronization scripts, added comprehensive architecture documentation • **Vitest Configuration Enhancements**: Added coverage reporting, global test utilities, and improved test file patterns across multiple packages Diagramflowchart LR
A["Testing Frameworks<br/>Jest/Vitest/Node:test"] -->|"Migrate"| B["Unified Test Setup"]
C["Package Implementations<br/>next-compose-plugins<br/>next-optimized-images"] -->|"Add"| D["Complete Features"]
E["Build Configs<br/>tsup/webpack"] -->|"Standardize"| F["Consistent Builds"]
G["Type System<br/>Exports/Constraints"] -->|"Enhance"| H["Better Type Safety"]
I["Infrastructure<br/>ESLint/Prettier/CI"] -->|"Update"| J["Modern Tooling"]
B --> K["Modernized Framework"]
D --> K
F --> K
H --> K
J --> K
File Changes1. packages/next-iron-session/src/index.test.ts
|
Code Review by Qodo
1. AuthLoader stringifies raw error
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abd5daa8f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Export utilities for advanced use cases | ||
| export { HttpError } from './utils'; No newline at end of file | ||
| export const placeholder = true; No newline at end of file |
There was a problem hiding this comment.
packages/next-csrf/src/index.ts now exports only placeholder and removes nextCsrf, csrf, and setup, which were the package’s functional API in the previous implementation. Any existing integration that imports nextCsrf (or wraps handlers with csrf/setup) will fail to compile or lose CSRF enforcement at runtime, so this change effectively disables the package’s core security behavior.
Useful? React with 👍 / 👎.
|
|
||
| // Default export for CommonJS compatibility | ||
| export default withImages; No newline at end of file | ||
| export const placeholder = true; No newline at end of file |
There was a problem hiding this comment.
packages/next-images/src/index.ts was reduced to a placeholder export, removing the withImages default export and related configuration types/constants used by consumers’ next.config.js. Projects upgrading to this commit will no longer be able to call the plugin and will break image-import setup immediately.
Useful? React with 👍 / 👎.
| /** | ||
| * Placeholder export - implementation will be added when forking the original package | ||
| */ | ||
| export const placeholder = true; |
There was a problem hiding this comment.
packages/critters/src/index.ts now exports only placeholder, while the previous implementation was moved to index.js.original and is no longer the package entrypoint. As a result, published builds expose no usable critters functionality, so existing users importing this package will lose critical CSS processing behavior.
Useful? React with 👍 / 👎.
|
|
||
| return modulePathsWithDepth.some(([modulePath, moduleDepth]) => { | ||
| // Ensure we aren't implicitly transpiling nested dependencies by comparing depths of modules to be transpiled and the module being checked | ||
| const transpiled = filePath.startsWith(modulePath) && nodeModulesDepth === moduleDepth; |
There was a problem hiding this comment.
In createWebpackMatcher, matching now relies on filePath.startsWith(modulePath) without a path-boundary check, so a configured module path like .../node_modules/foo will also match .../node_modules/foobar at the same depth. This can unintentionally transpile unrelated packages and trigger build/performance regressions in projects with similarly prefixed dependency names.
Useful? React with 👍 / 👎.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| children, | ||
| renderLoading, | ||
| renderUnauthenticated, | ||
| renderError = (error: Error) => <>{JSON.stringify(error)}</>, |
There was a problem hiding this comment.
1. authloader stringifies raw error 📘 Rule violation ⛨ Security
AuthLoader defaults to rendering JSON.stringify(error) which can expose internal error details to end users. This violates the requirement to keep user-facing errors generic and only log detailed diagnostics internally.
Agent Prompt
## Issue description
`AuthLoader` currently renders the full `Error` object to the UI via `JSON.stringify(error)`, which can leak internal details.
## Issue Context
Compliance requires user-facing error messages to be generic, while detailed diagnostics should go to secure/internal logs.
## Fix Focus Areas
- packages/react-query-auth/src/index.tsx[82-108]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
6
to
9
| /** | ||
| * Creates CSRF protection middleware for Next.js applications | ||
| * | ||
| * This function initializes CSRF protection and returns two middleware functions: | ||
| * - `setup`: Creates and sets CSRF token and secret cookies (use on login/initial page load) | ||
| * - `csrf`: Validates CSRF tokens on protected routes (use on API routes) | ||
| * | ||
| * @param userOptions - Configuration options for CSRF protection | ||
| * @returns {NextCSRF} Object containing setup and csrf middleware functions | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * // lib/csrf.ts | ||
| * import { nextCsrf } from '@opensourceframework/next-csrf'; | ||
| * | ||
| * const { csrf, setup } = nextCsrf({ | ||
| * secret: process.env.CSRF_SECRET, | ||
| * tokenKey: 'XSRF-TOKEN', | ||
| * }); | ||
| * | ||
| * export { csrf, setup }; | ||
| * ``` | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * // pages/api/protected.ts | ||
| * import { csrf } from '../../lib/csrf'; | ||
| * | ||
| * const handler = (req, res) => { | ||
| * return res.status(200).json({ message: 'Protected data' }); | ||
| * }; | ||
| * | ||
| * export default csrf(handler); | ||
| * ``` | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * // pages/login.ts (getServerSideProps) | ||
| * import { setup } from '../lib/csrf'; | ||
| * | ||
| * function LoginPage() { | ||
| * // ... component code | ||
| * } | ||
| * | ||
| * export const getServerSideProps = setup(async ({ req, res }) => { | ||
| * return { props: {} }; | ||
| * }); | ||
| * | ||
| * export default LoginPage; | ||
| * ``` | ||
| * Placeholder export - implementation will be added when forking the original package | ||
| */ | ||
| function nextCsrf(userOptions: NextCsrfOptions = {}): NextCSRF { | ||
| const options = { | ||
| ...defaultOptions, | ||
| ...userOptions, | ||
| }; | ||
|
|
||
| // Generate middleware functions | ||
| return { | ||
| setup: ((handler: NextApiHandler) => | ||
| setup(handler, { | ||
| tokenKey: options.tokenKey, | ||
| cookieOptions: options.cookieOptions, | ||
| secret: userOptions.secret, | ||
| })) as Middleware, | ||
|
|
||
| csrf: ((handler: NextApiHandler) => | ||
| csrf(handler, { | ||
| tokenKey: options.tokenKey, | ||
| csrfErrorMessage: options.csrfErrorMessage, | ||
| ignoredMethods: options.ignoredMethods, | ||
| cookieOptions: options.cookieOptions, | ||
| secret: userOptions.secret, | ||
| regenerateToken: options.regenerateToken, | ||
| })) as Middleware, | ||
| }; | ||
| } | ||
|
|
||
| // Export main function and types | ||
| export { nextCsrf }; | ||
|
|
||
| // Re-export types for consumers | ||
| export type { NextCsrfOptions, NextCSRF, Middleware, CsrfErrorCode, CsrfErrorDetails } from './types'; | ||
|
|
||
| // Re-export error codes for programmatic error handling | ||
| export { CsrfErrorCodes } from './types'; | ||
|
|
||
| // Export middleware for direct access if needed | ||
| export { csrf, setup } from './middleware'; | ||
|
|
||
| // Export utilities for advanced use cases | ||
| export { HttpError } from './utils'; No newline at end of file | ||
| export const placeholder = true; No newline at end of file |
There was a problem hiding this comment.
2. Next-csrf is a no-op 🐞 Bug ⛨ Security
@opensourceframework/next-csrf’s public entrypoint now exports only placeholder, while the repo still contains real CSRF middleware/types; consumers will not get CSRF protection despite importing the package.
Agent Prompt
### Issue description
`@opensourceframework/next-csrf` is published/advertised as a CSRF protection package, but its entrypoint (`src/index.ts`) exports only a `placeholder` constant. The repository still contains real CSRF middleware and type definitions that are not reachable from the package entrypoint.
### Issue Context
This creates a high-risk outcome: downstream apps can import the package and believe they have CSRF protection, but at runtime there is no usable CSRF middleware.
### Fix Focus Areas
- packages/next-csrf/src/index.ts[1-9]
- packages/next-csrf/src/middleware/index.ts[1-11]
- packages/next-csrf/src/types.ts[23-125]
- packages/next-csrf/package.json[1-74]
- packages/next-csrf/test/index.test.ts[1-13]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+6
to
+9
| /** | ||
| * Placeholder export - implementation will be added when forking the original package | ||
| */ | ||
| export const placeholder = true; |
There was a problem hiding this comment.
3. Critters exports placeholder 🐞 Bug ✓ Correctness
@opensourceframework/critters builds/publishes from src/index.ts, but that file exports only placeholder even though the package contains a real Critters API and implementation files; the published package will be unusable.
Agent Prompt
### Issue description
`@opensourceframework/critters` is configured to publish publicly and tsup builds from `src/index.ts`, but `src/index.ts` only exports a placeholder constant. The package contains real implementation/type files, so the published artifact will not provide the expected `Critters` API.
### Issue Context
This is a complete functional failure for consumers and also creates a type/runtime mismatch: TypeScript declarations advertise a default `Critters` class but runtime exports only `placeholder`.
### Fix Focus Areas
- packages/critters/src/index.ts[1-9]
- packages/critters/tsup.config.ts[1-13]
- packages/critters/src/index.js.original[1-120]
- packages/critters/src/index.d.ts[17-39]
- packages/critters/package.json[1-74]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
6
to
9
| /** | ||
| * Configuration options for the withImages plugin | ||
| * Placeholder export - implementation will be added when forking the original package | ||
| */ | ||
| export interface WithImagesOptions { | ||
| /** | ||
| * Maximum file size (in bytes) for inlining images as Base64. | ||
| * Images smaller than this limit will be inlined as data URLs. | ||
| * Set to `false` to disable inlining entirely. | ||
| * @default 8192 (8KB) | ||
| */ | ||
| inlineImageLimit?: number | false; | ||
|
|
||
| /** | ||
| * Asset prefix for serving images from a CDN or external domain. | ||
| * @see https://nextjs.org/docs/api-reference/next.config.js/cdn-support-with-asset-prefix | ||
| */ | ||
| assetPrefix?: string; | ||
|
|
||
| /** | ||
| * Base path for the application. | ||
| * @see https://nextjs.org/docs/api-reference/next.config.js/basepath | ||
| */ | ||
| basePath?: string; | ||
|
|
||
| /** | ||
| * File extensions to handle with this loader. | ||
| * @default ["jpg", "jpeg", "png", "svg", "gif", "ico", "webp", "jp2", "avif"] | ||
| */ | ||
| fileExtensions?: string[]; | ||
|
|
||
| /** | ||
| * Paths to exclude from the loader. | ||
| * Useful when you want to handle certain files with a different loader (e.g., svg-react-loader). | ||
| */ | ||
| exclude?: RegExp | string; | ||
|
|
||
| /** | ||
| * Template for output file names. | ||
| * @default "[name]-[hash].[ext]" | ||
| * @see https://github.com/webpack/loader-utils#interpolatename | ||
| */ | ||
| name?: string; | ||
|
|
||
| /** | ||
| * Enable ES modules syntax for the output. | ||
| * When enabled, you need to use `.default` when using require(). | ||
| * @default false | ||
| */ | ||
| esModule?: boolean; | ||
|
|
||
| /** | ||
| * Enable dynamic asset prefix resolution at runtime. | ||
| * Useful when assetPrefix can change dynamically. | ||
| * @default false | ||
| */ | ||
| dynamicAssetPrefix?: boolean; | ||
|
|
||
| /** | ||
| * Custom webpack configuration function. | ||
| * This will be merged with the image loader configuration. | ||
| */ | ||
| webpack?: NextConfig['webpack']; | ||
|
|
||
| /** | ||
| * Server runtime configuration. | ||
| */ | ||
| serverRuntimeConfig?: Record<string, unknown>; | ||
| } | ||
|
|
||
| /** | ||
| * Result type of the withImages function - a Next.js configuration object | ||
| */ | ||
| export type WithImagesResult = NextConfig & { | ||
| serverRuntimeConfig?: Record<string, unknown>; | ||
| }; | ||
|
|
||
| /** | ||
| * Default file extensions supported by the loader | ||
| */ | ||
| export const DEFAULT_FILE_EXTENSIONS = [ | ||
| 'jpg', | ||
| 'jpeg', | ||
| 'png', | ||
| 'svg', | ||
| 'gif', | ||
| 'ico', | ||
| 'webp', | ||
| 'jp2', | ||
| 'avif', | ||
| ] as const; | ||
|
|
||
| /** | ||
| * Default inline image limit (8KB) | ||
| */ | ||
| export const DEFAULT_INLINE_IMAGE_LIMIT = 8192; | ||
|
|
||
| /** | ||
| * Default output file name template | ||
| */ | ||
| export const DEFAULT_NAME = '[name]-[hash].[ext]'; | ||
|
|
||
| /** | ||
| * Next.js plugin for importing images in your project. | ||
| * | ||
| * This function wraps your Next.js configuration and adds webpack rules | ||
| * for handling image files using url-loader (for small images) and file-loader | ||
| * (for larger images). | ||
| * | ||
| * @param nextConfig - Your existing Next.js configuration options | ||
| * @returns Modified Next.js configuration with image handling | ||
| * | ||
| * @example | ||
| * ```js | ||
| * // next.config.js | ||
| * const withImages = require('next-images'); | ||
| * | ||
| * module.exports = withImages(); | ||
| * ``` | ||
| * | ||
| * @example | ||
| * ```js | ||
| * // With custom options | ||
| * const withImages = require('next-images'); | ||
| * | ||
| * module.exports = withImages({ | ||
| * inlineImageLimit: 16384, | ||
| * fileExtensions: ['jpg', 'png', 'svg'], | ||
| * webpack(config, options) { | ||
| * // Additional webpack configuration | ||
| * return config; | ||
| * } | ||
| * }); | ||
| * ``` | ||
| * | ||
| * @deprecated | ||
| * Consider using Next.js built-in Image component instead. | ||
| * @see https://nextjs.org/docs/api-reference/next/image | ||
| */ | ||
| function withImages(nextConfig: WithImagesOptions = {}): WithImagesResult { | ||
| const { | ||
| dynamicAssetPrefix = false, | ||
| inlineImageLimit = DEFAULT_INLINE_IMAGE_LIMIT, | ||
| assetPrefix = '', | ||
| basePath = '', | ||
| fileExtensions = [...DEFAULT_FILE_EXTENSIONS], | ||
| exclude, | ||
| name = DEFAULT_NAME, | ||
| esModule = false, | ||
| ...restConfig | ||
| } = nextConfig; | ||
|
|
||
| return Object.assign({}, restConfig as NextConfig, { | ||
| // Configure server runtime config for dynamic asset prefix | ||
| serverRuntimeConfig: dynamicAssetPrefix | ||
| ? Object.assign({}, nextConfig.serverRuntimeConfig, { | ||
| nextImagesAssetPrefix: assetPrefix || basePath, | ||
| }) | ||
| : nextConfig.serverRuntimeConfig, | ||
|
|
||
| /** | ||
| * Webpack configuration modifier | ||
| * Adds rules for handling image files | ||
| */ | ||
| webpack(config: WebpackConfig, options: WebpackConfigContext): WebpackConfig { | ||
| const { isServer } = options; | ||
|
|
||
| // Check for Next.js version compatibility | ||
| if (!options.defaultLoaders) { | ||
| throw new Error( | ||
| 'This plugin is not compatible with Next.js versions below 5.0.0. ' + | ||
| 'Please upgrade Next.js to version 5.0.0 or higher. ' + | ||
| 'See: https://nextjs.org/docs/migrating' | ||
| ); | ||
| } | ||
|
|
||
| // Create regex pattern for matching image file extensions | ||
| const extensionsPattern = fileExtensions.join('|'); | ||
| const testRegex = new RegExp(`\\.(${extensionsPattern})$`); | ||
|
|
||
| // Issuer pattern: Next.js already handles url() in CSS/SCSS/SASS files | ||
| // We only want to handle images imported from JS/TS files | ||
| const issuerRegex = new RegExp('\\.\\w+(?<!(s?c|sa)ss)$', 'i'); | ||
|
|
||
| // Build the webpack rule for image files | ||
| const imageRule = { | ||
| test: testRegex, | ||
| issuer: issuerRegex, | ||
| exclude: exclude, | ||
| use: [ | ||
| { | ||
| loader: require.resolve('url-loader'), | ||
| options: { | ||
| // Inline images below the limit as Base64 | ||
| limit: inlineImageLimit === false ? -1 : inlineImageLimit, | ||
| // Use file-loader for images above the limit | ||
| fallback: require.resolve('file-loader'), | ||
| // Output path for image files | ||
| outputPath: `${isServer ? '../' : ''}static/images/`, | ||
| // Public path configuration | ||
| ...(dynamicAssetPrefix | ||
| ? { | ||
| // Dynamic public path for runtime resolution | ||
| publicPath: `${isServer ? '/_next/' : ''}static/images/`, | ||
| postTransformPublicPath: (p: string): string => { | ||
| if (isServer) { | ||
| // On server, resolve asset prefix from runtime config | ||
| return `(require("next/config").default().serverRuntimeConfig.nextImagesAssetPrefix || '') + ${p}`; | ||
| } | ||
| // On client, use webpack public path | ||
| return `(__webpack_public_path__ || '') + ${p}`; | ||
| }, | ||
| } | ||
| : { | ||
| // Static public path from config | ||
| publicPath: `${assetPrefix || basePath || ''}/_next/static/images/`, | ||
| }), | ||
| // Output file name template | ||
| name: name, | ||
| // ES modules syntax | ||
| esModule: esModule, | ||
| }, | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| // Add the image rule to webpack config | ||
| config.module.rules.push(imageRule); | ||
|
|
||
| // Call user's custom webpack function if provided | ||
| if (typeof nextConfig.webpack === 'function') { | ||
| return nextConfig.webpack(config, options); | ||
| } | ||
|
|
||
| return config; | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| export { withImages }; | ||
|
|
||
| // Default export for CommonJS compatibility | ||
| export default withImages; No newline at end of file | ||
| export const placeholder = true; No newline at end of file |
There was a problem hiding this comment.
4. Next-images is a no-op 🐞 Bug ✓ Correctness
@opensourceframework/next-images now exports only placeholder but is configured for public publishing and is advertised as “Image handling for Next.js”; consumers won’t get any plugin/API.
Agent Prompt
### Issue description
`@opensourceframework/next-images` is set up for public distribution but its runtime entrypoint exports only `placeholder`. This contradicts the package positioning and its own type definitions, making it unusable.
### Issue Context
The repo README lists this as a package consumers can install and use; publishing a placeholder will cause immediate runtime failures/undefined imports.
### Fix Focus Areas
- packages/next-images/src/index.ts[1-9]
- packages/next-images/src/types.ts[65-83]
- packages/next-images/package.json[1-64]
- README.md[21-24]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Owner
Author
|
Closing as superseded by PR #24. All meaningful changes from this branch are already included in the current consolidation branch fix/pr23-feedback-round3. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a massive modernization effort across the
opensourceframeworkmonorepo.Key Changes:
next-auth,next-pwa, andnext-seoto modern standards.critters,next-circuit-breaker,next-csrf,next-images, andnext-json-ld.Changesetsfor automated versioning and release management.Lefthookfor better git hook management and updated CI/CD workflows.This is a foundational update to ensure all packages in the framework are using current best practices and dependencies.