Skip to content
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

upgrade rollup out of the stone age #480

Merged
merged 3 commits into from
Mar 8, 2024
Merged

Conversation

madhav-stripe
Copy link
Contributor

@madhav-stripe madhav-stripe commented Mar 5, 2024

Summary & motivation

In this PR we are upgrading rollup and our build configurations to the most recent / modern versions of the libraries. I will do my best to explain what has been upgraded. I've added comments inline to where the changes have been made

image
st-madhav1:react-stripe-js madhav$ npx publint
@stripe/react-stripe-js lint results:
Suggestions:
1. The package does not specify the type field. Environments may incorrectly identify a CJS file as ESM in the future. Consider adding "type": "commonjs".
2. pkg.module is used to output ESM, but pkg.exports is not defined. As NodeJS doesn't read pkg.module, the ESM output may be skipped. Consider adding pkg.exports to export the ESM output. pkg.module can usually be removed alongside too. (This will be a breaking change)
3. pkg["jsnext:main"] is deprecated. pkg.module should be used instead.

API review

Copy this template or link to an API review issue.

Testing & documentation

@madhav-stripe madhav-stripe changed the title upgrade rollup get rollup out of the stone age Mar 5, 2024
@madhav-stripe madhav-stripe changed the title get rollup out of the stone age upgrade rollup out of the stone age Mar 5, 2024
@@ -11,7 +11,7 @@ jobs:
- uses: actions/checkout@v2
- uses: actions/setup-node@v1
with:
node-version: 14.x
node-version: 20.x
Copy link
Contributor Author

Choose a reason for hiding this comment

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

modern versions of rollup do not support node < 18. Since Node.js v20 is available and LTS, we are upgrading the action runner to use Node v20

@@ -3,8 +3,8 @@
"version": "2.5.1",
"description": "React components for Stripe.js and Stripe Elements",
"main": "dist/react-stripe.js",
"module": "dist/react-stripe.esm.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran npx publint and it gave the suggestion to rename the exports here from js to mjs.

When consuming this package there might be some ambiguity onto wether this file is ESM or Common JS (because it ends with .js). Changing the file extension to .mjs explicitly calls out that this files uses ESM

@@ -14,7 +14,7 @@
"lint": "eslint --max-warnings=0 '{src,examples}/**/*.{ts,tsx,js}'",
"lint:prettier": "prettier './**/*.js' './**/*.ts' './**/*.tsx' './**/*.css' './**/*.md' --list-different",
"typecheck": "tsc",
"build": "yarn run clean && yarn run rollup -c && yarn checkimport",
"build": "yarn run clean && yarn run rollup -c --bundleConfigAsCjs && yarn checkimport",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the model version of rollup expects the rollup config to be in ESM (if using the .js) syntax. To not have a larger diff than needed - passing this flag loads a .js config as CommonJS

package.json Outdated
@@ -54,13 +54,18 @@
}
},
"dependencies": {
"@rollup/plugin-babel": "^6.0.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the previous versions of these plugins were named rollup-plug-xxxx. Rollup has since upgraded and changed the name of these plugins to the @rollup/plugin-xxx naming convention.

@@ -71,7 +76,6 @@
"@types/react-dom": "^18.0.0",
"@typescript-eslint/eslint-plugin": "^2.18.0",
"@typescript-eslint/parser": "^2.18.0",
"@wessberg/rollup-plugin-ts": "^1.2.15",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The author of this plugin has published new versions of this plugin as rollup-plugin-ts

"ts-jest": "^25.1.0",
"ts-loader": "^6.2.1",
"typescript": "^3.7.5"
"typescript": "^4.1.2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

upgrading typescript here to match the typescript version in stripe-js

@madhav-stripe madhav-stripe marked this pull request as ready for review March 5, 2024 20:03
@madhav-stripe
Copy link
Contributor Author

@@ -61,6 +61,11 @@
"@babel/core": "^7.7.2",
"@babel/preset-env": "^7.7.1",
"@babel/preset-react": "^7.7.0",
"@rollup/plugin-babel": "^6.0.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the older versions of rollup plugins were in the format rollup-plugin-xxxxx, it has now changed to `@rollup/plugin-xxxxx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants