Conversation
carlbrugger
force-pushed
the
feat/bundlers
branch
4 times, most recently
from
5bd02bb to
78e598d
Compare
carlbrugger
force-pushed
the
feat/bundlers
branch
from
780ca93 to
3ad014f
Compare
carlbrugger
force-pushed
the
feat/bundlers
branch
from
d27f1c0 to
449301d
Compare
WalkthroughThis pull request introduces a minor version update for multiple Flatfile plugins and utilities, including extractors, converters, and validation tools. Notable changes include the transition of the build process from Rollup to tsup, affecting how plugins are built and packaged. Additionally, several Rollup configuration files have been removed, and corresponding tsup configuration files have been added across various plugins, establishing a new structure for bundling configurations. Changes
Possibly related PRs
Suggested reviewers
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (6)
plugins/merge-connection/src/index.ts (1)
1-1: Consider updating documentation and adding migration guide.
Since this is a breaking change in the export pattern, ensure that:
- Package documentation is updated to reflect the new import pattern
- A migration guide is provided for users updating to the new version
Would you like me to help draft the documentation updates or migration guide?
plugins/constraints/src/external.constraint.ts (1)
33-33: LGTM! Consider enhancing error context.
The hook implementation is correct and maintains the same functionality. However, we could improve the error context for better debugging.
Consider enhancing the error handling to include more context:
- record.addError(property.key, String(e))
+ record.addError(property.key, `Validation '${validator}' failed: ${String(e)}`).changeset/five-hounds-wash.md (1)
60-61: Enhance release notes with more details about the bundling improvements.
The current note is too brief and doesn't capture the full scope of changes. Consider expanding it to highlight:
- The dual bundler setup (rollup + tsup)
- The new bundle formats (ESM, CJS, UMD)
- The bundling issues that were fixed
Here's a suggested revision:
-The release swaps the package's bundler to tsup.
+This release implements a dual bundler setup using rollup and tsup to:
+- Generate browser and Node.js compatible bundles in ESM, CJS, and UMD formats
+- Fix bundling issues including masquerading as ESM, resolution failures, and missing type definitions
+- Improve bundle validation using @arethetypeswrong/cli and publintutils/testing/src/test.listener.ts (1)
Line range hint 58-96: Consider adding promise cleanup mechanism.
While not directly related to the current changes, the waitFor method creates promises that might remain pending if the expected event count is never reached. Consider adding a timeout or cleanup mechanism to prevent potential memory leaks in long-running tests.
Here's a suggested improvement:
waitFor(
topic: string,
neededCount: number = 1,
- filter?: EventFilter | string
+ filter?: EventFilter | string,
+ timeoutMs: number = 30000
): Promise<number> {
if (typeof filter === 'string') {
filter = { topic, job: filter }
} else {
filter = { topic, ...filter }
}
- return new Promise((resolver) => {
+ return new Promise((resolver, reject) => {
+ const timeoutId = setTimeout(() => {
+ const index = this.invocationWatchers.findIndex(w => w.resolver === resolver);
+ if (index !== -1) {
+ this.invocationWatchers.splice(index, 1);
+ }
+ reject(new Error(`Timeout waiting for ${topic} after ${timeoutMs}ms`));
+ }, timeoutMs);
const pastInvocations = this.invocations.get(topic) || []
const executedCount = pastInvocations.filter((event) =>
this.matchEvent(event, filter)
).length
this.invocationWatchers.push({
filter,
neededCount,
executedCount,
- resolver,
+ resolver: (result) => {
+ clearTimeout(timeoutId);
+ resolver(result);
+ },
})bundlers/rollup-config/index.mjs (1)
46-67: Consider enabling sourcemaps consistently across formats.
The ESM build explicitly disables sourcemaps while the CJS build doesn't specify this option. Consider being consistent with sourcemap generation across formats.
{
exports: 'auto',
file: 'dist/index.cjs',
format: 'cjs',
+ sourcemap: false,
},plugins/delimiter-extractor/src/parser.ts (1)
19-21: LGTM! Explicit default handling improves type safety.
The null coalescing operator addition makes the default behavior explicit and handles both undefined and null cases consistently.
Consider adding a type annotation to make the boolean nature of this option even more explicit:
- : (options?.skipEmptyLines ?? false)
+ : (options?.skipEmptyLines ?? false) as boolean📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (62)
bundlers/rollup-config/package.jsonis excluded by!**/*.jsonbundlers/tsup-config/package.jsonis excluded by!**/*.jsonconvert/currency/package.jsonis excluded by!**/*.jsonconvert/translate/package.jsonis excluded by!**/*.jsonconvert/what3words/package.jsonis excluded by!**/*.jsonenrich/geocode/package.jsonis excluded by!**/*.jsonenrich/gpx/package.jsonis excluded by!**/*.jsonenrich/sentiment/package.jsonis excluded by!**/*.jsonenrich/summarize/package.jsonis excluded by!**/*.jsonexport/pivot-table/package.jsonis excluded by!**/*.jsonextract/html-table/package.jsonis excluded by!**/*.jsonextract/markdown/package.jsonis excluded by!**/*.jsonimport/faker/package.jsonis excluded by!**/*.jsonimport/llm-records/package.jsonis excluded by!**/*.jsonimport/rss/package.jsonis excluded by!**/*.jsonpackage-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.jsonplugins/autocast/package.jsonis excluded by!**/*.jsonplugins/automap/package.jsonis excluded by!**/*.jsonplugins/constraints/package.jsonis excluded by!**/*.jsonplugins/dedupe/package.jsonis excluded by!**/*.jsonplugins/delimiter-extractor/package.jsonis excluded by!**/*.jsonplugins/dxp-configure/package.jsonis excluded by!**/*.jsonplugins/export-workbook/package.jsonis excluded by!**/*.jsonplugins/foreign-db-extractor/package.jsonis excluded by!**/*.jsonplugins/graphql-schema/package.jsonis excluded by!**/*.jsonplugins/job-handler/package.jsonis excluded by!**/*.jsonplugins/json-extractor/package.jsonis excluded by!**/*.jsonplugins/json-schema/package.jsonis excluded by!**/*.jsonplugins/merge-connection/package.jsonis excluded by!**/*.jsonplugins/openapi-schema/package.jsonis excluded by!**/*.jsonplugins/pdf-extractor/package.jsonis excluded by!**/*.jsonplugins/psv-extractor/package.jsonis excluded by!**/*.jsonplugins/record-hook/package.jsonis excluded by!**/*.jsonplugins/rollout/package.jsonis excluded by!**/*.jsonplugins/space-configure/package.jsonis excluded by!**/*.jsonplugins/sql-ddl-converter/package.jsonis excluded by!**/*.jsonplugins/tsv-extractor/package.jsonis excluded by!**/*.jsonplugins/view-mapped/package.jsonis excluded by!**/*.jsonplugins/webhook-egress/package.jsonis excluded by!**/*.jsonplugins/webhook-event-forwarder/package.jsonis excluded by!**/*.jsonplugins/xlsx-extractor/package.jsonis excluded by!**/*.jsonplugins/xml-extractor/package.jsonis excluded by!**/*.jsonplugins/xml-extractor/tsconfig.jsonis excluded by!**/*.jsonplugins/yaml-schema/package.jsonis excluded by!**/*.jsonplugins/zip-extractor/package.jsonis excluded by!**/*.jsonsupport/common-utils/package.jsonis excluded by!**/*.jsontsconfig.jsonis excluded by!**/*.jsonturbo.jsonis excluded by!**/*.jsonutils/common/package.jsonis excluded by!**/*.jsonutils/extractor/package.jsonis excluded by!**/*.jsonutils/fetch-schema/package.jsonis excluded by!**/*.jsonutils/file-buffer/package.jsonis excluded by!**/*.jsonutils/response-rejection/package.jsonis excluded by!**/*.jsonutils/testing/package.jsonis excluded by!**/*.jsonvalidate/boolean/package.jsonis excluded by!**/*.jsonvalidate/date/package.jsonis excluded by!**/*.jsonvalidate/email/package.jsonis excluded by!**/*.jsonvalidate/isbn/package.jsonis excluded by!**/*.jsonvalidate/number/package.jsonis excluded by!**/*.jsonvalidate/phone/package.jsonis excluded by!**/*.jsonvalidate/string/package.jsonis excluded by!**/*.json
📒 Files selected for processing (106)
- .changeset/five-hounds-wash.md (1 hunks)
- bundlers/rollup-config/CHANGELOG.md (1 hunks)
- bundlers/rollup-config/index.mjs (3 hunks)
- bundlers/tsup-config/index.mjs (1 hunks)
- convert/currency/rollup.config.mjs (0 hunks)
- convert/currency/tsup.config.mjs (1 hunks)
- convert/translate/rollup.config.mjs (0 hunks)
- convert/translate/tsup.config.mjs (1 hunks)
- convert/what3words/rollup.config.mjs (0 hunks)
- convert/what3words/tsup.config.mjs (1 hunks)
- enrich/geocode/rollup.config.mjs (0 hunks)
- enrich/geocode/tsup.config.mjs (1 hunks)
- enrich/gpx/rollup.config.mjs (0 hunks)
- enrich/gpx/tsup.config.mjs (1 hunks)
- enrich/sentiment/rollup.config.mjs (0 hunks)
- enrich/sentiment/tsup.config.mjs (1 hunks)
- enrich/summarize/rollup.config.mjs (0 hunks)
- enrich/summarize/tsup.config.mjs (1 hunks)
- export/pivot-table/rollup.config.mjs (0 hunks)
- export/pivot-table/tsup.config.mjs (1 hunks)
- extract/html-table/rollup.config.mjs (0 hunks)
- extract/html-table/tsup.config.mjs (1 hunks)
- extract/markdown/rollup.config.mjs (0 hunks)
- extract/markdown/tsup.config.mjs (1 hunks)
- flatfilers/sandbox/src/index.ts (1 hunks)
- import/faker/rollup.config.mjs (0 hunks)
- import/faker/tsup.config.mjs (1 hunks)
- import/llm-records/rollup.config.mjs (0 hunks)
- import/llm-records/tsup.config.mjs (1 hunks)
- import/rss/rollup.config.mjs (0 hunks)
- import/rss/tsup.config.mjs (1 hunks)
- plugins/autocast/rollup.config.mjs (0 hunks)
- plugins/autocast/src/autocast.plugin.ts (2 hunks)
- plugins/autocast/tsup.config.mjs (1 hunks)
- plugins/automap/tsup.config.mjs (1 hunks)
- plugins/constraints/rollup.config.mjs (0 hunks)
- plugins/constraints/src/external.constraint.ts (2 hunks)
- plugins/constraints/src/external.sheet.constraint.ts (2 hunks)
- plugins/constraints/tsup.config.mjs (1 hunks)
- plugins/dedupe/rollup.config.mjs (0 hunks)
- plugins/dedupe/tsup.config.mjs (1 hunks)
- plugins/delimiter-extractor/src/parser.ts (2 hunks)
- plugins/delimiter-extractor/tsup.config.mjs (1 hunks)
- plugins/dxp-configure/rollup.config.mjs (0 hunks)
- plugins/dxp-configure/tsup.config.mjs (1 hunks)
- plugins/export-workbook/rollup.config.mjs (0 hunks)
- plugins/export-workbook/tsup.config.mjs (1 hunks)
- plugins/foreign-db-extractor/rollup.config.mjs (0 hunks)
- plugins/foreign-db-extractor/tsup.config.mjs (1 hunks)
- plugins/graphql-schema/rollup.config.mjs (0 hunks)
- plugins/graphql-schema/tsup.config.mjs (1 hunks)
- plugins/job-handler/rollup.config.mjs (0 hunks)
- plugins/job-handler/tsup.config.mjs (1 hunks)
- plugins/json-extractor/rollup.config.mjs (0 hunks)
- plugins/json-extractor/tsup.config.mjs (1 hunks)
- plugins/json-schema/tsup.config.mjs (1 hunks)
- plugins/merge-connection/src/index.ts (1 hunks)
- plugins/merge-connection/src/merge.plugin.ts (2 hunks)
- plugins/merge-connection/tsup.config.mjs (1 hunks)
- plugins/openapi-schema/rollup.config.mjs (0 hunks)
- plugins/openapi-schema/tsup.config.mjs (1 hunks)
- plugins/pdf-extractor/tsup.config.mjs (1 hunks)
- plugins/psv-extractor/tsup.config.mjs (1 hunks)
- plugins/record-hook/rollup.config.mjs (2 hunks)
- plugins/record-hook/tsup.config.mjs (1 hunks)
- plugins/rollout/tsup.config.mjs (1 hunks)
- plugins/space-configure/rollup.config.mjs (0 hunks)
- plugins/space-configure/tsup.config.mjs (1 hunks)
- plugins/sql-ddl-converter/tsup.config.mjs (1 hunks)
- plugins/tsv-extractor/tsup.config.mjs (1 hunks)
- plugins/view-mapped/rollup.config.mjs (0 hunks)
- plugins/view-mapped/tsup.config.mjs (1 hunks)
- plugins/webhook-egress/rollup.config.mjs (0 hunks)
- plugins/webhook-egress/tsup.config.mjs (1 hunks)
- plugins/webhook-event-forwarder/rollup.config.mjs (0 hunks)
- plugins/webhook-event-forwarder/tsup.config.mjs (1 hunks)
- plugins/xlsx-extractor/tsup.config.mjs (1 hunks)
- plugins/xml-extractor/tsup.config.mjs (1 hunks)
- plugins/yaml-schema/tsup.config.mjs (1 hunks)
- plugins/zip-extractor/tsup.config.mjs (1 hunks)
- support/common-utils/tsup.config.mjs (1 hunks)
- utils/common/rollup.config.mjs (0 hunks)
- utils/common/tsup.config.mjs (1 hunks)
- utils/extractor/tsup.config.mjs (1 hunks)
- utils/fetch-schema/tsup.config.mjs (1 hunks)
- utils/file-buffer/tsup.config.mjs (1 hunks)
- utils/response-rejection/rollup.config.mjs (0 hunks)
- utils/response-rejection/tsup.config.mjs (1 hunks)
- utils/testing/src/test.helpers.ts (0 hunks)
- utils/testing/src/test.listener.ts (1 hunks)
- utils/testing/tsup.config.mjs (1 hunks)
- validate/boolean/rollup.config.mjs (0 hunks)
- validate/boolean/tsup.config.mjs (1 hunks)
- validate/date/rollup.config.mjs (0 hunks)
- validate/date/tsup.config.mjs (1 hunks)
- validate/email/rollup.config.mjs (0 hunks)
- validate/email/tsup.config.mjs (1 hunks)
- validate/isbn/rollup.config.mjs (0 hunks)
- validate/isbn/tsup.config.mjs (1 hunks)
- validate/number/rollup.config.mjs (0 hunks)
- validate/number/tsup.config.mjs (1 hunks)
- validate/phone/rollup.config.mjs (0 hunks)
- validate/phone/tsup.config.mjs (1 hunks)
- validate/string/jest.config.cjs (1 hunks)
- validate/string/rollup.config.mjs (0 hunks)
- validate/string/tsup.config.mjs (1 hunks)
💤 Files with no reviewable changes (37)
- convert/currency/rollup.config.mjs
- convert/translate/rollup.config.mjs
- convert/what3words/rollup.config.mjs
- enrich/geocode/rollup.config.mjs
- enrich/gpx/rollup.config.mjs
- enrich/sentiment/rollup.config.mjs
- enrich/summarize/rollup.config.mjs
- export/pivot-table/rollup.config.mjs
- extract/html-table/rollup.config.mjs
- extract/markdown/rollup.config.mjs
- import/faker/rollup.config.mjs
- import/llm-records/rollup.config.mjs
- import/rss/rollup.config.mjs
- plugins/autocast/rollup.config.mjs
- plugins/constraints/rollup.config.mjs
- plugins/dedupe/rollup.config.mjs
- plugins/dxp-configure/rollup.config.mjs
- plugins/export-workbook/rollup.config.mjs
- plugins/foreign-db-extractor/rollup.config.mjs
- plugins/graphql-schema/rollup.config.mjs
- plugins/job-handler/rollup.config.mjs
- plugins/json-extractor/rollup.config.mjs
- plugins/openapi-schema/rollup.config.mjs
- plugins/space-configure/rollup.config.mjs
- plugins/view-mapped/rollup.config.mjs
- plugins/webhook-egress/rollup.config.mjs
- plugins/webhook-event-forwarder/rollup.config.mjs
- utils/common/rollup.config.mjs
- utils/response-rejection/rollup.config.mjs
- utils/testing/src/test.helpers.ts
- validate/boolean/rollup.config.mjs
- validate/date/rollup.config.mjs
- validate/email/rollup.config.mjs
- validate/isbn/rollup.config.mjs
- validate/number/rollup.config.mjs
- validate/phone/rollup.config.mjs
- validate/string/rollup.config.mjs
✅ Files skipped from review due to trivial changes (57)
- bundlers/rollup-config/CHANGELOG.md
- convert/currency/tsup.config.mjs
- convert/translate/tsup.config.mjs
- convert/what3words/tsup.config.mjs
- enrich/geocode/tsup.config.mjs
- enrich/gpx/tsup.config.mjs
- enrich/sentiment/tsup.config.mjs
- enrich/summarize/tsup.config.mjs
- export/pivot-table/tsup.config.mjs
- extract/html-table/tsup.config.mjs
- extract/markdown/tsup.config.mjs
- import/faker/tsup.config.mjs
- import/llm-records/tsup.config.mjs
- import/rss/tsup.config.mjs
- plugins/autocast/tsup.config.mjs
- plugins/automap/tsup.config.mjs
- plugins/constraints/tsup.config.mjs
- plugins/dedupe/tsup.config.mjs
- plugins/delimiter-extractor/tsup.config.mjs
- plugins/dxp-configure/tsup.config.mjs
- plugins/export-workbook/tsup.config.mjs
- plugins/foreign-db-extractor/tsup.config.mjs
- plugins/graphql-schema/tsup.config.mjs
- plugins/job-handler/tsup.config.mjs
- plugins/json-extractor/tsup.config.mjs
- plugins/json-schema/tsup.config.mjs
- plugins/merge-connection/tsup.config.mjs
- plugins/openapi-schema/tsup.config.mjs
- plugins/pdf-extractor/tsup.config.mjs
- plugins/psv-extractor/tsup.config.mjs
- plugins/record-hook/tsup.config.mjs
- plugins/rollout/tsup.config.mjs
- plugins/space-configure/tsup.config.mjs
- plugins/sql-ddl-converter/tsup.config.mjs
- plugins/tsv-extractor/tsup.config.mjs
- plugins/view-mapped/tsup.config.mjs
- plugins/webhook-egress/tsup.config.mjs
- plugins/webhook-event-forwarder/tsup.config.mjs
- plugins/xlsx-extractor/tsup.config.mjs
- plugins/xml-extractor/tsup.config.mjs
- plugins/yaml-schema/tsup.config.mjs
- plugins/zip-extractor/tsup.config.mjs
- support/common-utils/tsup.config.mjs
- utils/common/tsup.config.mjs
- utils/extractor/tsup.config.mjs
- utils/fetch-schema/tsup.config.mjs
- utils/file-buffer/tsup.config.mjs
- utils/response-rejection/tsup.config.mjs
- utils/testing/tsup.config.mjs
- validate/boolean/tsup.config.mjs
- validate/date/tsup.config.mjs
- validate/email/tsup.config.mjs
- validate/isbn/tsup.config.mjs
- validate/number/tsup.config.mjs
- validate/phone/tsup.config.mjs
- validate/string/jest.config.cjs
- validate/string/tsup.config.mjs
🔇 Additional comments (17)
plugins/merge-connection/src/index.ts (1)
1-1: Breaking change: Export style modification requires consumer updates.
The change from default export to namespace export is technically correct and aligns with modern ESM practices. However, this is a breaking change that will require updates to import statements in consuming code.
Let's verify the impact on existing imports:
✅ Verification successful
Export style change appears to be safe
Since no imports of @flatfile/plugin-merge-connection were found in the codebase, this export style modification appears to be an internal change that doesn't affect any existing code. The package might be:
- A new package that hasn't been used yet
- Used only by external consumers
- Being prepared for first release
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for existing imports of mergePlugin to identify affected code
# Expected: Find import statements that need to be updated
# Search for default imports
echo "Checking for default imports that need updating..."
rg "import\s+(\w+\s+from\s+)?['\"]@flatfile/plugin-merge-connection['\"]" -g '!{node_modules,dist}/*'
# Search for existing namespace imports (should be none if this is a new pattern)
echo "Checking for existing namespace imports..."
rg "import\s*{\s*mergePlugin\s*}\s*from\s*['\"]@flatfile/plugin-merge-connection['\"]" -g '!{node_modules,dist}/*'
Length of output: 606
Script:
#!/bin/bash
# Let's try a simpler search pattern to find any imports of the package
echo "Searching for any imports of the package..."
rg "@flatfile/plugin-merge-connection" -g '!{node_modules,dist}/*'
# Also check package.json files for dependencies
echo "Checking package.json files for dependencies..."
fd "package.json" --type f --exec grep -l "@flatfile/plugin-merge-connection" {}
Length of output: 349
plugins/record-hook/rollup.config.mjs (1)
11-13: Configuration might not meet PR objectives.
The current configuration appears to contradict the PR's stated objectives:
- PR aims to produce browser and Node.js compatible bundles, but both
includeNodeandincludeBrowserare set tofalse includeDefinitions: falsemight affect type definitions, which the PR aims to fix- Only UMD bundle is enabled while PR mentions ESM and CJS formats
Please verify if:
- These bundles are being generated elsewhere
- This plugin specifically needs only UMD format
- Type definitions are handled by another configuration
plugins/merge-connection/src/merge.plugin.ts (1)
6-6: LGTM! Export pattern aligns with modern ESM best practices.
The change from default export to named export improves compatibility with both ESM and CommonJS environments, enabling better tree-shaking capabilities.
plugins/constraints/src/external.constraint.ts (1)
4-4: LGTM! Import changes align with bundling improvements.
The updated import structure better supports both ESM and CJS environments.
Let's verify the package exports are correctly configured:
✅ Verification successful
Import changes are correctly implemented and exported
The verification confirms that:
- The
bulkRecordHookexport is properly defined in@flatfile/plugin-record-hookpackage (aliased frombulkRecordHookPlugin) - The import is widely used across the codebase in a consistent manner
- The package is a direct dependency in multiple projects, including the constraints plugin
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify @flatfile/plugin-record-hook package exports
publint @flatfile/plugin-record-hook
# Check for any type mismatches in the package
at-least-node @flatfile/plugin-record-hook
Length of output: 239
Script:
#!/bin/bash
# Let's verify the imports by checking the actual source code
# First, find the package location and check its exports
fd -t f "package.json" | xargs rg "@flatfile/plugin-record-hook"
# Check for actual usage of bulkRecordHook in the codebase
rg "bulkRecordHook" -A 2 -B 2
# Look for type definitions or implementation of these imports
ast-grep --pattern 'export { $$ as bulkRecordHook }'
ast-grep --pattern 'export const bulkRecordHook'
Length of output: 18440
plugins/constraints/src/external.sheet.constraint.ts (2)
42-42: LGTM! Implementation maintains functional equivalence.
The update to use bulkRecordHook is consistent with the import change and preserves the existing behavior.
5-5: Verify the package compatibility across module formats.
The change from bulkRecordHookPlugin to bulkRecordHook appears to be part of standardizing the API. Let's verify the new export is available in all bundle formats.
✅ Verification successful
Let me generate additional scripts to verify the API changes more thoroughly.
This will help us verify:
- The implementation details of the record-hook plugin
- Any remaining imports of the old API
- The proper export of the new API name
API change is properly implemented and safe to use
The change from bulkRecordHookPlugin to bulkRecordHook is a safe refactoring as the original function is re-exported with the new name in the record-hook plugin. The implementation shows:
export {
bulkRecordHookPlugin as bulkRecordHook,
recordHookPlugin as recordHook,
}This ensures backward compatibility while standardizing the API naming convention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the availability of bulkRecordHook in the package exports
# Test 1: Check if the package.json properly exports the function
echo "Checking package exports..."
fd -e json -x jq -r '.exports' {} \; 2>/dev/null
# Test 2: Search for any remaining usage of the old API name
echo "Checking for old API usage..."
rg "bulkRecordHookPlugin" -l
Length of output: 26542
Script:
#!/bin/bash
# Check the implementation and exports in record-hook plugin
echo "Checking record-hook plugin implementation..."
cat plugins/record-hook/src/record.hook.plugin.ts
# Check if there are any other files importing bulkRecordHookPlugin
echo -e "\nChecking imports of old API..."
rg "import.*bulkRecordHookPlugin" -l
# Check if the new API is properly exported
echo -e "\nChecking new API exports..."
rg "export.*bulkRecordHook" -A 2
Length of output: 1681
.changeset/five-hounds-wash.md (1)
1-58: LGTM! Package version bumps are comprehensive.
The minor version bumps for all packages align with the scope of bundler changes. Both @flatfile/bundler-config-rollup and @flatfile/bundler-config-tsup are included, supporting the dual bundler setup mentioned in the PR objectives.
utils/testing/src/test.listener.ts (2)
32-32: LGTM! Good TypeScript practice.
Adding the override keyword improves type safety by ensuring the method signature matches the parent class FlatfileListener. This is a TypeScript best practice that helps catch potential inheritance-related bugs at compile time.
Line range hint 1-5: Verify module compatibility with new bundling setup.
Since this PR introduces dual ESM/CJS bundle support, we should ensure this testing utility works correctly in both module systems, particularly the imports from '@flatfile/api' and '@flatfile/listener'.
Let's verify the package exports and imports:
✅ Verification successful
Let me gather more information about the package configuration and imports.
Let me check the workspace package configuration since this is a monorepo setup.
Module compatibility is properly configured
The package is correctly set up for dual ESM/CJS support with:
- Proper
exportsfield defining both ESM and CJS entry points - Separate type definitions for both module systems
- Explicit browser and node targets
- Using
tsupbundler with dual-format configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify package.json configuration for proper dual module support
# Check if package.json properly defines both ESM and CJS entry points
echo "Checking package.json configuration..."
jq -r '.exports, .main, .module, .type' package.json
# Verify that @flatfile dependencies are configured for dual module support
echo "Checking @flatfile dependencies configuration..."
for dep in api listener; do
if [ -f "node_modules/@flatfile/${dep}/package.json" ]; then
echo "Checking @flatfile/${dep}..."
jq -r '.exports, .main, .module, .type' "node_modules/@flatfile/${dep}/package.json"
fi
done
Length of output: 431
Script:
#!/bin/bash
# Check the actual package.json content to understand the project setup
echo "Package.json content:"
cat package.json
# Check if there are any tsconfig.json files that might indicate module configuration
echo -e "\nTS Config files:"
fd tsconfig.json
# Look for build configuration files
echo -e "\nBuild config files:"
fd -e rollup.config.js -e webpack.config.js
# Check the actual imports in the test listener file
echo -e "\nTest listener imports:"
cat utils/testing/src/test.listener.ts
Length of output: 5532
Script:
#!/bin/bash
# Check the testing utils package configuration
echo "Testing utils package.json:"
cat utils/testing/package.json
# Check tsconfig for module settings
echo -e "\nTesting utils tsconfig:"
cat utils/testing/tsconfig.json 2>/dev/null || echo "No direct tsconfig found"
# Check if there's a build config in the testing utils
echo -e "\nBuild configuration:"
ls utils/testing/*.config.* 2>/dev/null || echo "No direct build config found"
# Check the dist folder structure if it exists
echo -e "\nDist folder structure (if exists):"
ls utils/testing/dist 2>/dev/null || echo "No dist folder found"
Length of output: 2470
bundlers/rollup-config/index.mjs (2)
92-100: LGTM: Type definitions generation looks good.
The conditional type definitions generation addresses the "missing type definitions" issue mentioned in the PR objectives.
108-108: LGTM: Clear UMD bundle naming.
The .umd.js suffix clearly identifies the bundle format.
flatfilers/sandbox/src/index.ts (2)
1-2: LGTM! Type-only import improves bundle optimization.
The addition of the type modifier for the Flatfile import is a good practice. This change ensures better tree-shaking by explicitly marking type-only imports, which helps reduce the final bundle size.
4-7: LGTM! Verify type-only imports don't affect runtime.
The addition of type modifiers for configuration interfaces is correct. Let's verify that these changes don't affect the runtime behavior of the plugin.
✅ Verification successful
Let me gather more information to verify the type imports and exports properly.
Based on the search results, I can now generate the final response:
Type imports are correctly used and don't affect runtime behavior
The search results confirm that:
PartialSheetConfigandPartialWorkbookConfigare properly defined as interfaces/types across multiple plugins- These types are consistently used as type annotations for configuration objects
- The types are only used at compile-time for type checking and don't affect runtime behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the types are properly handled in the build output
# and that the runtime functionality is preserved.
# Test 1: Check if the types are properly stripped in the output
echo "Checking build output for type imports..."
rg -l "import.*type.*PartialSheetConfig" --type ts
# Test 2: Verify that configureSpaceWithJsonSchema is still exported as a value
echo "Verifying value exports..."
ast-grep --pattern 'export { configureSpaceWithJsonSchema }'
Length of output: 271
Script:
#!/bin/bash
# Check how PartialSheetConfig and PartialWorkbookConfig are used in the codebase
echo "Checking usage of configuration types..."
rg "PartialSheetConfig|PartialWorkbookConfig" -B 2 -A 2
# Check the actual export declarations for these types
echo "Checking type exports in source files..."
ast-grep --pattern 'export type PartialSheetConfig'
ast-grep --pattern 'export type PartialWorkbookConfig'
# Verify configureSpaceWithJsonSchema implementation
echo "Checking configureSpaceWithJsonSchema implementation..."
ast-grep --pattern 'export function configureSpaceWithJsonSchema'
Length of output: 15449
plugins/delimiter-extractor/src/parser.ts (2)
6-6: LGTM! Import statement aligns with modern bundling practices.
The simplified import syntax better supports dual ESM/CJS compatibility, which aligns with the PR's bundling objectives.
Line range hint 1-5: Verify bundle compatibility with external dependencies.
Given the PR's focus on bundling improvements, we should verify:
- Proper bundling of external dependencies (Papa, remeda)
- Type definitions availability
- Browser compatibility when using Node.js built-ins (stream)
✅ Verification successful
Based on the package.json files and dependencies analysis, I can now generate a final response regarding the bundling concerns:
Bundle configuration is properly set up for browser compatibility
The plugin is using the recommended bundling setup with proper configurations:
- Uses
tsupbundler (via@flatfile/bundler-config-tsup) which handles Node.js built-ins correctly - Has proper export conditions for both Node.js and browser environments in package.json:
"exports": {
"node": { ... },
"browser": { ... }
}- External dependencies (Papa, remeda) are properly declared as dependencies and will be bundled
- Stream usage is properly handled with browser fallbacks via export conditions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all dependencies are properly declared and will be bundled
publint
# Verify type definitions
@arethetypeswrong/cli check ./dist
# Check for Node.js built-in usage that might affect browser compatibility
rg -l 'require|import.*["\']stream["\']' --type ts
Length of output: 312
Script:
#!/bin/bash
# Check package.json for dependencies and build configuration
cat package.json
# Check for Node.js built-in imports across TypeScript files
rg "from ['\"](stream|buffer|fs|path|crypto)" --type ts
# Look for bundler configuration files
fd -e rollup.config.js -e webpack.config.js -e vite.config.ts
# Check for type declaration files
fd "\.d\.ts$"
# Look for build output patterns in package.json and tsconfig
fd "(tsconfig.*|package)\.json$" --exec cat {}
Length of output: 118971
plugins/autocast/src/autocast.plugin.ts (2)
21-21: Function signature change looks good.
The update to use bulkRecordHook maintains the same parameter structure and usage pattern, ensuring backward compatibility.
5-5: Verify module compatibility across different bundle formats.
The import change from bulkRecordHookPlugin to bulkRecordHook aligns with the PR's goal of fixing bundling issues. Let's verify this change works across all bundle formats.
✅ Verification successful
Import change is a valid alias renaming
The verification shows that bulkRecordHook is actually an alias for bulkRecordHookPlugin in the @flatfile/plugin-record-hook package. This is confirmed by the export statement:
export {
bulkRecordHookPlugin as bulkRecordHook,
recordHookPlugin as recordHook,
}The change in import statement is part of a standardization effort and maintains full compatibility since both names refer to the same function. The function is properly exported and used consistently across multiple plugins in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the availability and export of bulkRecordHook in different bundle formats
# Check if the module properly exports bulkRecordHook
ast-grep --pattern 'export { $$ as bulkRecordHook }'
# Look for any conditional exports or package.json export maps
rg -l "bulkRecordHook" package.json
Length of output: 90
Script:
#!/bin/bash
# Let's try a different approach to verify the import changes
# Check for exports of bulkRecordHook in any form
ast-grep --pattern 'export $$ bulkRecordHook'
# Search for the actual function definition and its usage
rg -A 2 "bulkRecordHook" --type ts
# Check the plugin-record-hook package for exports
fd -e ts -e js "index" -x cat {} \; | rg "bulkRecordHook|export"
# Look for the original bulkRecordHookPlugin to understand the change
rg "bulkRecordHookPlugin" --type ts
Length of output: 11586
| const EXTENSION_MAP = { | ||
| browser: { | ||
| cjs: '.browser.cjs', | ||
| default: '.browser.js', | ||
| }, | ||
| node: { | ||
| cjs: '.cjs', | ||
| default: '.js', | ||
| }, | ||
| } | ||
| const getOutExtension = | ||
| (platform) => | ||
| ({ format }) => ({ | ||
| js: EXTENSION_MAP[platform][format] || EXTENSION_MAP[platform].default, | ||
| }) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety to extension mapping.
The extension mapping logic could benefit from TypeScript types and validation for unknown formats.
// Add these type definitions at the top of the file
type Platform = 'browser' | 'node'
type Format = 'cjs' | 'default'
type ExtensionMap = Record<Platform, Record<Format, string>>
const EXTENSION_MAP: ExtensionMap = {
// ... your existing map
}
const getOutExtension = (platform: Platform) =>
({ format }: { format: string }) => ({
js: EXTENSION_MAP[platform][format as Format] ?? EXTENSION_MAP[platform].default
})| import dotenv from 'dotenv' | ||
| import { defineConfig as tsupDefineConfig } from 'tsup' | ||
|
|
||
| dotenv.config() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for environment configuration.
The direct call to dotenv.config() might fail silently if the .env file is missing. Consider adding error handling and making the environment configuration more explicit.
import dotenv from 'dotenv'
import { defineConfig as tsupDefineConfig } from 'tsup'
-dotenv.config()
+try {
+ dotenv.config({ debug: process.env.DEBUG === 'true' })
+} catch (error) {
+ console.warn('Warning: Error loading .env file:', error.message)
+}📝 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.
| import dotenv from 'dotenv' | |
| import { defineConfig as tsupDefineConfig } from 'tsup' | |
| dotenv.config() | |
| import dotenv from 'dotenv' | |
| import { defineConfig as tsupDefineConfig } from 'tsup' | |
| try { | |
| dotenv.config({ debug: process.env.DEBUG === 'true' }) | |
| } catch (error) { | |
| console.warn('Warning: Error loading .env file:', error.message) | |
| } |
| const createConfig = (platform) => ({ | ||
| name: platform, | ||
| platform, | ||
| minify, | ||
| entryPoints: ['src/index.ts'], | ||
| format: ['cjs', 'esm'], | ||
| dts: true, | ||
| outDir: 'dist', | ||
| clean: true, | ||
| sourcemap: true, | ||
| treeshake: true, | ||
| splitting: true, | ||
| outExtension: getOutExtension(platform), | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance configuration flexibility.
The configuration could be more flexible by:
- Allowing customization of entry points
- Supporting external dependencies configuration
- Validating entry point existence
-const createConfig = (platform) => ({
+const createConfig = (platform, options = {}) => ({
name: platform,
platform,
minify,
- entryPoints: ['src/index.ts'],
+ entryPoints: options.entryPoints ?? ['src/index.ts'],
format: ['cjs', 'esm'],
dts: true,
outDir: 'dist',
clean: true,
sourcemap: true,
treeshake: true,
splitting: true,
outExtension: getOutExtension(platform),
+ external: options.external ?? [],
})📝 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.
| const createConfig = (platform) => ({ | |
| name: platform, | |
| platform, | |
| minify, | |
| entryPoints: ['src/index.ts'], | |
| format: ['cjs', 'esm'], | |
| dts: true, | |
| outDir: 'dist', | |
| clean: true, | |
| sourcemap: true, | |
| treeshake: true, | |
| splitting: true, | |
| outExtension: getOutExtension(platform), | |
| }) | |
| const createConfig = (platform, options = {}) => ({ | |
| name: platform, | |
| platform, | |
| minify, | |
| entryPoints: options.entryPoints ?? ['src/index.ts'], | |
| format: ['cjs', 'esm'], | |
| dts: true, | |
| outDir: 'dist', | |
| clean: true, | |
| sourcemap: true, | |
| treeshake: true, | |
| splitting: true, | |
| outExtension: getOutExtension(platform), | |
| external: options.external ?? [], | |
| }) |
| export function defineConfig({ includeBrowser = true, includeNode = true }) { | ||
| const minify = process.env.NODE_ENV === 'production' | ||
| if (!minify) { | ||
| console.log('Not in production mode - skipping minification') | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve environment validation and logging.
The current implementation has two potential improvements:
- Console.log statements should not be present in production code
- NODE_ENV validation could be more robust
export function defineConfig({ includeBrowser = true, includeNode = true }) {
- const minify = process.env.NODE_ENV === 'production'
- if (!minify) {
- console.log('Not in production mode - skipping minification')
+ const nodeEnv = process.env.NODE_ENV?.toLowerCase()
+ if (!['development', 'production', 'test'].includes(nodeEnv)) {
+ console.warn(`Warning: Unexpected NODE_ENV value: ${nodeEnv}. Defaulting to development.`)
}
+ const minify = nodeEnv === 'production'📝 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.
| export function defineConfig({ includeBrowser = true, includeNode = true }) { | |
| const minify = process.env.NODE_ENV === 'production' | |
| if (!minify) { | |
| console.log('Not in production mode - skipping minification') | |
| } | |
| export function defineConfig({ includeBrowser = true, includeNode = true }) { | |
| const nodeEnv = process.env.NODE_ENV?.toLowerCase() | |
| if (!['development', 'production', 'test'].includes(nodeEnv)) { | |
| console.warn(`Warning: Unexpected NODE_ENV value: ${nodeEnv}. Defaulting to development.`) | |
| } | |
| const minify = nodeEnv === 'production' |
| const nodeConfig = createConfig('node') | ||
| const browserConfig = createConfig('browser') | ||
|
|
||
| const configs = [] | ||
| if (includeNode) configs.push(nodeConfig) | ||
| if (includeBrowser) configs.push(browserConfig) | ||
|
|
||
| return tsupDefineConfig(configs) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Add validation to prevent empty configurations
The suggested validation is important since the config is widely used across the project (50+ files) and an empty configuration could cause build failures. Adding the validation will provide clear error messages instead of potential downstream build errors.
const configs = []
if (includeNode) configs.push(nodeConfig)
if (includeBrowser) configs.push(browserConfig)
+ if (configs.length === 0) {
+ throw new Error('At least one platform (browser or node) must be included in the configuration')
+ }
+
return tsupDefineConfig(configs)🔗 Analysis chain
Add configuration validation.
The configuration application should validate that at least one platform is included to prevent empty configurations.
const configs = []
if (includeNode) configs.push(nodeConfig)
if (includeBrowser) configs.push(browserConfig)
+ if (configs.length === 0) {
+ throw new Error('At least one platform (browser or node) must be included in the configuration')
+ }
+
return tsupDefineConfig(configs)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this configuration is used consistently across the project
rg -l "from ['\"]\@flatfile\/bundler-config-tsup['\"]"
Length of output: 180
Script:
#!/bin/bash
# Search for imports of the tsup config package with proper escaping
rg -l "from ['\"]@flatfile/bundler-config-tsup['\"]"
# Search for direct usage of the package name
rg -l "@flatfile/bundler-config-tsup"
# Look for package.json files that might use this dependency
fd "package.json" --exec grep -l "bundler-config-tsup" {}
Length of output: 7736
Script:
#!/bin/bash
# Check how the config is used in some of these files to understand if validation exists in usage
rg -A 5 "from ['\"]\@flatfile\/bundler-config-tsup['\"]" "plugins/*/tsup.config.mjs" "utils/*/tsup.config.mjs"
# Look at the implementation to see if there's any existing validation
cat bundlers/tsup-config/index.mjs
Length of output: 1477
Script:
#!/bin/bash
# Let's check how this config is used in some example files with proper escaping
rg -A 5 "from '@flatfile/bundler-config-tsup'" "plugins/*/tsup.config.mjs" "utils/*/tsup.config.mjs"
# Also check if there are any existing error handling patterns in the codebase
rg "throw new Error\(" bundlers/tsup-config/
Length of output: 285
| { | ||
| exports: 'auto', | ||
| file: 'dist/index.browser.mjs', | ||
| file: 'dist/index.browser.js', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider browser-specific optimizations.
While the browser build is properly separated, consider adding browser-specific optimizations such as:
- Tree-shaking configuration
- Browser-specific external dependencies
- Compression options
| includeNode = true, | ||
| includeBrowser = true, | ||
| includeDefinition = true, | ||
| includeUmd = false, | ||
| umdConfig = { name: undefined, external: [] }, |
There was a problem hiding this comment.
💡 Codebase verification
Rollup configuration should be removed as tsup migration is complete
The codebase shows clear evidence of a successful migration to tsup:
- 50+ packages have already migrated to tsup configurations
- Only one Rollup configuration remains at
plugins/record-hook/rollup.config.mjs, which also has a corresponding tsup configuration
Since the package plugins/record-hook has both Rollup and tsup configurations, and given the PR's objective to transition to tsup, this Rollup configuration file should be removed to maintain consistency across the codebase.
🔗 Analysis chain
Verify the transition plan from Rollup to tsup.
While the parameter additions improve bundle configuration flexibility, the PR objectives indicate a transition from Rollup to tsup. Please clarify if this file will be maintained alongside tsup or if it's intended to be deprecated.
Let's verify the transition status:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for tsup configuration files and their usage
echo "Searching for tsup configurations..."
rg -l "defineConfig.*from.*@flatfile/bundler-config-tsup"
echo "Checking for remaining Rollup configurations..."
fd -e config.js -e config.mjs rollup
Length of output: 2268
2 participants
Please explain how to summarize this PR for the Changelog:
This PR changes the build process to use a combination of
rollupandtsupto build browser and node compatible ESM and CJS bundles as well as a UMD bundle. Also integrates@arethetypeswrong/cliandpublintto check for bundling issues as well as fixes the issues found using these tools so that they are all 🟢. No more:Tell code reviewer how and what to test:
Everything 😆