-
Notifications
You must be signed in to change notification settings - Fork 100
feat: new esm builds #2822
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
base: master
Are you sure you want to change the base?
feat: new esm builds #2822
Conversation
| AppActionCall, | ||
| AppActionCallProps, | ||
| AppActionCallErrorProps, | ||
| AppActionCallResponse, // was previously deep imported in user_interface |
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.
exported a few types that weren't exported before but imported deeply in user_interface
| PlainClientAPI, | ||
| AppKeyProps, | ||
| } from '../../lib/contentful-management' | ||
| } from '../../lib/index' |
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.
all these test changes were importing from the main index file instead of contentful-management, which was renamed to index.ts
rollup.config.js
Outdated
| import commonjs from '@rollup/plugin-commonjs' | ||
| import json from '@rollup/plugin-json' | ||
| // import alias from '@rollup/plugin-alias' | ||
| // import terser from '@rollup/plugin-terser' |
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.
all the commented out code here was in support of creating the browser bundles, which I didn't do initially to save time, and I want to bring it up to the group if this is even worth keeping
types.d.ts
Outdated
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 file was how the types were exported before. It was included as a part of the bundle at the root and allowed imports from 'contentful-management/types'
rollup.config.js
Outdated
| @@ -0,0 +1,221 @@ | |||
| // import { resolve, dirname } from 'path' | |||
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 file needs to be .mjs to work for me
rollup.config.js
Outdated
| }), | ||
| json(), | ||
| ], | ||
| external: [/node_modules\/(?!tslib.*)/], |
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.
| external: [/node_modules\/(?!tslib.*)/], | |
| external: ['contentful-sdk-core', 'fast-copy', 'axios'], |
Did this regex work for you? For me, they are not prefixed with node_modules.
When going with a simple array, it works well for me.
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.
When used in conjunction with nodeResolve, this will make it so no packages in node_modules (except tslib) get included in the bundle. Good precaution in case we add a new dependency and we don't remember to update these arrays.
rollup.config.js
Outdated
| __VERSION__: JSON.stringify(pkg.version), | ||
| }), | ||
| ], | ||
| external: [/node_modules\/(?!tslib.*)/], |
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.
| external: [/node_modules\/(?!tslib.*)/], | |
| external: baseConfig.external, |
Maybe using a extra variable for both spots makes more sense here
|
A few more things I found, and couldn't add a comment as review:
|
This pull request introduces significant changes to the build system, package exports, and type exports for the Contentful Management API client. The main focus is on migrating the build process from Babel/Webpack to Rollup, modernizing the package structure for better compatibility with modern bundlers, and improving the type exports for easier consumption. There are also updates to CI/CD workflows and deprecation notices.
Build System and Packaging Modernization:
rollup.config.jswith configurations for ESM, CommonJS bundles, and type declarations. This change streamlines the build process and improves compatibility with modern JavaScript tooling. [1] [2] [3] [4]package.jsonto use the newexportsfield for conditional exports (ESM, CJS, and types), set the new entry points, and removed legacy build scripts and fields. The minimum Node.js version is now 20. [1] [2] [3]Type Exports and API Surface Improvements:
AppActionCallResponse,CommentStatus,EntryReferenceProps,ActionType,ConstraintType,WebhookCallDetailsProps,WebhookCallOverviewProps,WebhookHealthProps) to be exported directly fromlib/export-types.ts, making them easier to import and improving DX. [1] [2] [3] [4] [5]OptionalDefaultsfromlib/plain/wrappers/wrap.tsin the main index, clarifying its intended usage as private and subject to change. [1] [2]Release and CI/CD Updates:
refs/heads/new-beta), supporting an additional prerelease channel. [1] [2]Most Important Changes:
Build System and Packaging:
rollup.config.js, providing ESM, CJS, and type declaration outputs, and updated related scripts and dependencies inpackage.json. [1] [2] [3] [4]package.jsonexports field for better bundler compatibility, updated entry points, and increased minimum Node.js version to 20. [1] [2]Type Exports and API Surface:
lib/export-types.ts, eliminating the need for deep imports and improving developer experience. [1] [2] [3] [4] [5]OptionalDefaultsin the main index and clarified its private usage in documentation. [1] [2]Deprecations and CI/CD:
createClientmethod, guiding users towards newer alternatives.new-beta). [1] [2]