-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate from Rollup
to preconstruct
for bundling
#1933
base: yarn-to-pnpm
Are you sure you want to change the base?
Conversation
|
Rollup
to preconstruct
for bundling
8bd40cf
to
94a8f7e
Compare
@@ -5,14 +5,6 @@ const getPresets = () => { | |||
targets: { node: 'current' }, | |||
modules: 'commonjs', | |||
} | |||
case 'rollup': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed anymore.
@@ -10,9 +10,8 @@ | |||
"postinstall": "./scripts/postinstall.sh", | |||
"clean": "rm -rf coverage; manypkg exec rimraf lib dist node_modules; rm -rf node_modules", | |||
"commit": "git-cz", | |||
"build": "pnpm --recursive run build", | |||
"build": "pnpm preconstruct build", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This builds what's configured below.
"packages/api-request-builder", | ||
"packages/http-user-agent", | ||
"packages/sdk-auth", | ||
"packages/sdk-client", | ||
"packages/sdk-middleware-auth", | ||
"packages/sdk-middleware-correlation-id", | ||
"packages/sdk-middleware-http", | ||
"packages/sdk-middleware-logger", | ||
"packages/sdk-middleware-queue", | ||
"packages/sdk-middleware-user-agent", | ||
"packages/sync-actions" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all packages which are configured as SDKs and were prior built with Rollup.
"main": "dist/commercetools-api-request-builder.cjs.js", | ||
"module": "dist/commercetools-api-request-builder.esm.js", | ||
"umd:main": "dist/commercetools-api-request-builder.umd.min.js", | ||
"preconstruct": { | ||
"entrypoints": [ | ||
"./index.js" | ||
], | ||
"umdName": "commercetoolsApiRequestBuilder" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming change is transparent to users it follows the preconstruct and general community standards.
libraryVersion?: string | ||
contactUrl?: string | ||
contactEmail?: string | ||
name: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise this is a syntax error.
"entrypoints": [ | ||
"./index.js" | ||
], | ||
"umdName": "commercetoolsSdkAuth" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just the function name in the UMD bundle.
@@ -2,10 +2,19 @@ | |||
|
|||
set -e | |||
|
|||
echo "Preparing development setup." | |||
if [ -n "$SKIP_POSTINSTALL" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This becomes a bit more elaborate and just as we have it on other repos.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## yarn-to-pnpm #1933 +/- ##
=============================================
Coverage 97.76% 97.76%
=============================================
Files 147 147
Lines 4779 4779
Branches 1277 1277
=============================================
Hits 4672 4672
Misses 104 104
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR migrates the bundling process from a custom Rollup configuration to preconstruct for the SDK while retaining Babel-based bundling for the CLI.
- Removed the Rollup preset from babel.config.js.
- Standardized TypeScript type definitions with semicolons in the user agent modules.
- Eliminated the rollup.config.js file, updating error handling in the auth flow to use the new errors import.
Reviewed Changes
File | Description |
---|---|
babel.config.js | Removed the Rollup-specific preset, streamlining bundling with preconstruct usage. |
packages/http-user-agent/src/create-user-agent.ts | Updated type definitions with consistent semicolon usage. |
packages/sdk-middleware-user-agent/src/user-agent.ts | Applied similar formatting updates for consistency. |
packages/sdk-middleware-auth/src/base-auth-flow.js | Revised error handling by switching to errors.NetworkError import. |
rollup.config.js | Entirely removed to complete migration to preconstruct-based bundling. |
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Summary
Simplifies the Rollup based bundling. We used a custom configuration so far and this is now replaced with preconstruct. For consumers this is not a noticeable change.
Preconstruct is used in many other packages we publish and uses Rollup under the hood.
Please note that this repo has two variations in bundling:
This Pull Request does not (yet) suggest to homogenize those variations.