-
Notifications
You must be signed in to change notification settings - Fork 3
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
Support React 19 #8
Conversation
…es - resolves all ATTW issues.
Tested manually that the bundle works both with React 18 and 19. |
@@ -2,7 +2,7 @@ | |||
|
|||
## react-clipboard-button | |||
|
|||
- [x] very light (~33KB not minified) | |||
- [x] very light (~1.2KB not minified) |
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.
by extenalizing the clipboard
dependency in vite config.
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.
Hey @RobinTail , thanks for the contribution!
Regarding this change, if I'm not mistaken, it means users will have to npm install clipboard
in addition to installing our library ? Any reason why you are removing the dependency from the bundle ?
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.
no, it will happen automatically, because clipboard
is listed as the production dependency, @guipas
Therefore it does not have to be bundled, because it's installed along with your packaage.
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.
react-clipboard-button/package.json
Lines 28 to 29 in 8e05720
"dependencies": { | |
"clipboard": "^2.0.8" |
Listing in dependencies
makes clipboard
installed into node_modules
on the consumer side along with this package. It's a subdependency accessible during the build on the consumer side. So you don't need to bundle clipboard
into react-clipboard-button
, @guipas
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.
users will have to npm install clipboard in addition
That would only be necessary if listing a dependency within peerDependencies
, where react
is listed in, for example, @guipas
Peer dependencies are not installed automatically.
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.
ok yeah that makes sense! thank you, I'll merge this PR
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.
I'll publish on npm later this week
@guipas , please review |
Fixes #7
Including general maintenance: upgrading all dependencies and adjusting some configs.
ATTW is also up to date and some adjustments made to the generated DTS files (now needs two of them, for each of the javascript bundles). This job was given to the Vite DTS plugin instead of
tsc
.Configuration of
extenals
is updated to avoid bundlingclipboard
dependency which is supposed to be installed on consumer side. That reduced the bundle size significantly.Tested manually: