Skip to content

Feat/bundlers #682

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

Merged
carlbrugger merged 20 commits into from
Oct 29, 2024
Merged

Feat/bundlers #682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
c5522c9
tsup vs rollup
Oct 23, 2024
0083eba
just tsup
Oct 23, 2024
fb8649f
new plugins -> tsup
Oct 23, 2024
dc63273
fix turbo.json
Oct 23, 2024
2d27e92
Fix package.json
Oct 23, 2024
06e21d8
jest.config.js --> jest.config.cjs
Oct 23, 2024
24262ec
add UMD support & remove parcel deps
Oct 24, 2024
294678b
Remove dup type
Oct 24, 2024
cfcd829
revert changelog update
Oct 24, 2024
d04c191
Create five-hounds-wash.md
Oct 24, 2024
1cdf87c
add publint
Oct 24, 2024
1799bf8
package.json updates
Oct 25, 2024
de08e1b
add umd build
Oct 25, 2024
eeebfd8
add sourcemaps & treeshaking
Oct 25, 2024
4f05aa7
Update index.mjs
Oct 25, 2024
44ff7b0
remove publint deps
Oct 25, 2024
1355083
add tsup config to missed packages
Oct 25, 2024
449301d
Fix main
Oct 28, 2024
cec9021
Fix dup devDep
Oct 29, 2024
2411464
cleanup
Oct 29, 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
61 changes: 61 additions & 0 deletions .changeset/five-hounds-wash.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
---
'@flatfile/plugin-webhook-event-forwarder': minor
'@flatfile/plugin-foreign-db-extractor': minor
'@flatfile/plugin-delimiter-extractor': minor
'@flatfile/plugin-convert-sql-ddl': minor
'@flatfile/plugin-connect-via-merge': minor
'@flatfile/util-response-rejection': minor
'@flatfile/plugin-export-workbook': minor
'@flatfile/plugin-space-configure': minor
'@flatfile/bundler-config-rollup': minor
'@flatfile/plugin-graphql-schema': minor
'@flatfile/plugin-json-extractor': minor
'@flatfile/plugin-convert-openapi-schema': minor
'@flatfile/plugin-webhook-egress': minor
'@flatfile/plugin-xlsx-extractor': minor
'@flatfile/plugin-dxp-configure': minor
'@flatfile/plugin-pdf-extractor': minor
'@flatfile/plugin-psv-extractor': minor
'@flatfile/plugin-tsv-extractor': minor
'@flatfile/plugin-xml-extractor': minor
'@flatfile/plugin-zip-extractor': minor
'@flatfile/bundler-config-tsup': minor
'@flatfile/common-plugin-utils': minor
'@flatfile/plugin-constraints': minor
'@flatfile/plugin-job-handler': minor
'@flatfile/plugin-convert-json-schema': minor
'@flatfile/plugin-record-hook': minor
'@flatfile/plugin-view-mapped': minor
'@flatfile/plugin-convert-yaml-schema': minor
'@flatfile/plugin-convert-what3words': minor
'@flatfile/plugin-export-pivot-table': minor
'@flatfile/plugin-extract-html-table': minor
'@flatfile/plugin-import-llm-records': minor
'@flatfile/util-fetch-schema': minor
'@flatfile/plugin-convert-translate': minor
'@flatfile/util-file-buffer': minor
'@flatfile/plugin-convert-currency': minor
'@flatfile/plugin-enrich-sentiment': minor
'@flatfile/plugin-enrich-summarize': minor
'@flatfile/plugin-extract-markdown': minor
'@flatfile/plugin-autocast': minor
'@flatfile/plugin-validate-boolean': minor
'@flatfile/plugin-automap': minor
'@flatfile/plugin-rollout': minor
'@flatfile/util-extractor': minor
'@flatfile/plugin-validate-number': minor
'@flatfile/plugin-validate-string': minor
'@flatfile/plugin-enrich-geocode': minor
'@flatfile/plugin-dedupe': minor
'@flatfile/plugin-validate-email': minor
'@flatfile/plugin-validate-phone': minor
'@flatfile/utils-testing': minor
'@flatfile/plugin-validate-date': minor
'@flatfile/plugin-validate-isbn': minor
'@flatfile/plugin-import-faker': minor
'@flatfile/util-common': minor
'@flatfile/plugin-enrich-gpx': minor
'@flatfile/plugin-import-rss': minor
---

The release swaps the package's bundler to tsup.
2 changes: 1 addition & 1 deletion bundlers/rollup-config/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# @flatfile/rollup-config
# @flatfile/bundler-config-rollup

## 0.1.1

Expand Down
60 changes: 35 additions & 25 deletions bundlers/rollup-config/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,30 +35,36 @@ function commonPlugins(browser, umd = false) {

export function buildConfig({
external = [],
includeNode = true,
includeBrowser = true,
includeDefinition = true,
includeUmd = false,
umdConfig = { name: undefined, external: [] },
Comment on lines +38 to 42

@coderabbitai coderabbitai Bot Oct 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 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

}) {
return [
// Node.js build
{
input: 'src/index.ts',
output: [
{
exports: 'auto',
file: 'dist/index.cjs',
format: 'cjs',
},
{
exports: 'auto',
file: 'dist/index.mjs',
sourcemap: false,
format: 'es',
},
],
plugins: commonPlugins(false),
external,
},
...(includeNode
? [
{
input: 'src/index.ts',
output: [
{
exports: 'auto',
file: 'dist/index.cjs',
format: 'cjs',
},
{
exports: 'auto',
file: 'dist/index.js',
sourcemap: false,
format: 'es',
},
],
plugins: commonPlugins(false),
external,
},
]
: []),
// Browser build
...(includeBrowser
? [
Expand All @@ -72,7 +78,7 @@ export function buildConfig({
},
{
exports: 'auto',
file: 'dist/index.browser.mjs',
file: 'dist/index.browser.js',

@coderabbitai coderabbitai Bot Oct 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

sourcemap: false,
format: 'es',
},
Expand All @@ -83,19 +89,23 @@ export function buildConfig({
]
: []),
// Definition file
{
input: 'src/index.ts',
output: [{ file: 'dist/index.d.ts', format: 'es' }],
plugins: [dts()],
},
...(includeDefinition
? [
{
input: 'src/index.ts',
output: [{ file: 'dist/index.d.ts', format: 'es' }],
plugins: [dts()],
},
]
: []),
// UMD build
...(includeUmd
? [
{
input: 'src/index.ts',
output: [
{
file: 'dist/index.js',
file: 'dist/index.umd.js',
format: 'umd',
name: umdConfig.name,
},
Expand Down
2 changes: 1 addition & 1 deletion bundlers/rollup-config/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"name": "@flatfile/rollup-config",
"name": "@flatfile/bundler-config-rollup",
"version": "0.1.1",
"description": "",
"private": true,
Expand Down
51 changes: 51 additions & 0 deletions bundlers/tsup-config/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import dotenv from 'dotenv'
import { defineConfig as tsupDefineConfig } from 'tsup'

dotenv.config()
Comment on lines +1 to +4

@coderabbitai coderabbitai Bot Oct 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
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)
}


export function defineConfig({ includeBrowser = true, includeNode = true }) {
const minify = process.env.NODE_ENV === 'production'
if (!minify) {
console.log('Not in production mode - skipping minification')
}
Comment on lines +6 to +10

@coderabbitai coderabbitai Bot Oct 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve environment validation and logging.

The current implementation has two potential improvements:

  1. Console.log statements should not be present in production code
  2. 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.

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

Comment on lines +12 to +27

@coderabbitai coderabbitai Bot Oct 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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),
})
Comment on lines +28 to +41

@coderabbitai coderabbitai Bot Oct 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance configuration flexibility.

The configuration could be more flexible by:

  1. Allowing customization of entry points
  2. Supporting external dependencies configuration
  3. 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.

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


const nodeConfig = createConfig('node')
const browserConfig = createConfig('browser')

const configs = []
if (includeNode) configs.push(nodeConfig)
if (includeBrowser) configs.push(browserConfig)

return tsupDefineConfig(configs)
}
Comment on lines +43 to +51

@coderabbitai coderabbitai Bot Oct 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 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

13 changes: 13 additions & 0 deletions bundlers/tsup-config/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"name": "@flatfile/bundler-config-tsup",
"version": "0.0.0",
"description": "",
"private": true,
"main": "index.mjs",
"scripts": {},
"author": "Flatfile, Inc.",
"license": "ISC",
"dependencies": {
"tsup": "^8.3.0"
}
}
0 convert/currency/jest.config.js → convert/currency/jest.config.cjs
View file
Open in desktop
File renamed without changes.
57 changes: 36 additions & 21 deletions convert/currency/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,50 @@
{
"name": "@flatfile/plugin-convert-currency",
"version": "0.1.0",
"url": "https://github.com/FlatFilers/flatfile-plugins/tree/main/convert/currency",
"description": "A Flatfile plugin for currency conversion using Open Exchange Rates API",
"main": "./dist/index.cjs",
"module": "./dist/index.mjs",
"types": "./dist/index.d.ts",
"browser": {
"./dist/index.cjs": "./dist/index.browser.cjs",
"./dist/index.mjs": "./dist/index.browser.mjs"
"registryMetadata": {
"category": "convert"
},
"type": "module",
"engines": {
"node": ">= 16"
},
"exports": {
"types": "./dist/index.d.ts",
"node": {
"import": "./dist/index.mjs",
"require": "./dist/index.cjs"
".": {
"node": {
"types": {
"import": "./dist/index.d.ts",
"require": "./dist/index.d.cts"
},
"import": "./dist/index.js",
"require": "./dist/index.cjs"
},
"browser": {
"types": {
"import": "./dist/index.d.ts",
"require": "./dist/index.d.cts"
},
"import": "./dist/index.browser.js",
"require": "./dist/index.browser.cjs"
},
"default": "./dist/index.js"
},
"browser": {
"require": "./dist/index.browser.cjs",
"import": "./dist/index.browser.mjs"
},
"default": "./dist/index.mjs"
"./package.json": "./package.json"
},
"main": "./dist/index.cjs",
"module": "./dist/index.js",
"source": "./src/index.ts",
"types": "./dist/index.d.ts",
"files": [
"dist/**"
],
"scripts": {
"build": "rollup -c",
"build:watch": "rollup -c --watch",
"build:prod": "NODE_ENV=production rollup -c",
"check": "tsc ./**/*.ts --noEmit --esModuleInterop",
"build": "tsup",
"build:watch": "tsup --watch",
"build:prod": "NODE_ENV=production tsup",
"checks": "tsc --noEmit && attw --pack . && publint .",
"lint": "tsc --noEmit",
"test": "jest src/*.spec.ts --detectOpenHandles",
"test:unit": "jest src/*.spec.ts --testPathIgnorePatterns=.*\\.e2e\\.spec\\.ts$ --detectOpenHandles",
"test:e2e": "jest src/*.e2e.spec.ts --detectOpenHandles"
Expand All @@ -49,11 +64,11 @@
"@flatfile/listener": "^1.0.1"
},
"devDependencies": {
"@flatfile/rollup-config": "^0.1.1"
"@flatfile/bundler-config-tsup": "^0.0.0"
},
"repository": {
"type": "git",
"url": "https://github.com/FlatFilers/flatfile-plugins.git",
"url": "git+https://github.com/FlatFilers/flatfile-plugins.git",
"directory": "convert/currency"
},
"browserslist": [
Expand Down
5 changes: 0 additions & 5 deletions convert/currency/rollup.config.mjs
View file
Open in desktop

This file was deleted.

3 changes: 3 additions & 0 deletions convert/currency/tsup.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { defineConfig } from '@flatfile/bundler-config-tsup'

export default defineConfig({})
0 convert/translate/jest.config.js → convert/translate/jest.config.cjs
View file
Open in desktop
File renamed without changes.
Loading