Skip to content

Fix: Jest expects commonjs module by default #98

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
merged 3 commits into from
Mar 8, 2022
Merged

Fix: Jest expects commonjs module by default #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7eeb9e
Move published build into dist directory
aburgel Feb 19, 2022
9a4853b
Fix reference to commonjs build
aburgel Feb 19, 2022
2d46fc2
Fix rollup build warnings
aburgel Feb 19, 2022
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
14 changes: 6 additions & 8 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@
"name": "sendbird-uikit",
"version": "2.6.0",
"description": "React based UI kit for sendbird",
"main": "release/index.js",
"style": "release/dist/index.css",
"typings": "release/index.d.ts",
"module": "release/index.js",
"main": "dist/cjs/index.js",
Copy link
Author

Choose a reason for hiding this comment

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

This fixes the jest issue.

I also moved the files into dist. I'm not sure why there was release and dist but it didn't seem necessary and was easy to clean up.

This should be backwards compatible since users shouldn't be referencing the release paths directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was due to some legacy customer request, but, can remove now :)

"style": "dist/index.css",
"typings": "dist/index.d.ts",
"module": "dist/index.js",
"files": [
"release/**/*",
"dist/**/*",
"package-lock.json",
"LICENSE",
Expand All @@ -19,9 +18,8 @@
"scripts": {
"test": "jest",
"test-update-snapshot": "jest -u",
"cp-css": "./scripts/copy-css.sh",
Copy link
Author

Choose a reason for hiding this comment

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

I cleaned up the rollup config so that this is no longer necessary. Rollup now outputs everything into dist.

"clean-release": "rm -rf ./release; rm -rf ./dist",
"build": "npm run clean-release; rollup -c; npm run cp-css",
"clean-release": "rm -rf ./dist",
"build": "npm run clean-release; rollup -c",
"start": "rollup -c -w",
"prepublishOnly": "npm run build",
"predeploy": "cd example && npm install && npm run build",
Expand Down
11 changes: 7 additions & 4 deletions rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,16 @@ module.exports = ({
},
output: [
{
dir: 'release/dist/cjs',
dir: 'dist/cjs',
format: 'cjs',
sourcemap: true,
exports: 'named'
Copy link
Author

Choose a reason for hiding this comment

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

This fixes a warning:

(!) Entry module "src/lib/Sendbird.jsx" is implicitly using "default" export mode, which means for CommonJS output that its default export is assigned to "module.exports". For many tools, such CommonJS output will not be interchangeable with the original ES module. If this is intended, explicitly set "output.exports" to either "auto" or "default", otherwise you might want to consider changing the signature of "src/lib/Sendbird.jsx" to use named exports only.

Copy link
Contributor

Choose a reason for hiding this comment

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

We would remove this, makes our scope a little wider

},
{
dir: 'release',
dir: 'dist',
format: 'esm',
sourcemap: true,
exports: 'named'
},
],
external: [
Expand All @@ -63,7 +65,7 @@ module.exports = ({
autoprefixer,
],
sourceMap: true,
extract: 'dist/index.css',
extract: 'index.css',
extensions: ['.sass', '.scss', '.css'],
}),
replace({
Expand Down Expand Up @@ -97,6 +99,7 @@ module.exports = ({
},
],
],
babelHelpers: 'bundled',
Copy link
Author

Choose a reason for hiding this comment

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

This fixes a warning:

babelHelpers: 'bundled' option was used by default. It is recommended to configure this option explicitly, read more here: https://github.com/rollup/plugins/tree/master/packages/babel#babelhelpers

extensions: ['.tsx', '.ts', '.jsx', '.js'],
exclude: 'node_modules/**',
plugins: [
Expand All @@ -114,7 +117,7 @@ module.exports = ({
targets: [
{
src: './src/index.d.ts',
dest: 'release',
dest: 'dist',
},
],
}),
Expand Down
5 changes: 0 additions & 5 deletions scripts/copy-css.sh
View file
Open in desktop

This file was deleted.