-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
v3: Refactored package structure / module exports #42
v3: Refactored package structure / module exports #42
Conversation
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.
Nice!
I'll fix up some of these things when I can, but as I'm traveling at the moment, might be a while. I will come back and go through the comments w/ implementations when I get the chance though. |
@ryanchristian4427 Seems to me that it's done. Pros:
Cons:
|
Just out of curiosity, where are you getting the numbers from? I'll certainly have a quick look myself too when I get the chance. |
Run |
Co-authored-by: Ryan Christian <[email protected]>
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.
Seems to me that we're done here.
Everything looks super good. I like the result!
Thanks @ryanchristian4427!
Quick demo of v3 (published a beta version): |
I'm little late to the party, but this looks good! import { HexColorPicker } from "../../src/"; Does that mean we're moving away from the default color picker component? I wonder if we could make an extra export named just |
Yeah, we moved away from default exports. It seems to me kinda wrong to mix named exports with the default one. I have heard that it could cause problems. So I would go with named exports only to avoid any issues in the future. I think the docs are clean enough. |
@omgovich Having no default exports is fine and I do understand why it's better. But would that still be possible to do: import { ColorPicker } from "react-colorful" (instead of importing a color picker specific to the color model, we could also provide a standard one, an alias to hex one, which will cover 90% of the use cases and potentially improve the experience of user who are just getting started with the library). |
Probably it's a good idea, but I'm also afraid that it might confuse people who read the docs. |
@ryanchristian4427 Do you have a Twitter account? |
@omgovich Actually, only just finally got around to making one. Nothing there yet, but I can be found at @_rschristian |
As the title states, this is for a v3 down the road. Definitely needs a lot of feedback, thought, and approval before then though.
I'll fill this out piece by piece, lot of documentation/justifications to write, but this is largely in response to #23 (comment) along with a few other scattered discussions.
Basic idea is less tooling, better API.
We'll let modern bundlers do the treeshaking rather than trying to do it ourselves. Microbundle doesn't really support what we currently have which can be seen by the duplicated CSS and TS definition files, along with the build script.
End User Changes (and why)
I propose a few changes for the end user, namely:
Instead of doing as we were with default exports of sub-modules, we could be using combined named exports for the entire module. For example,
becomes
A bit less verbose, which could be nice, but overall similar. Breaking change, and will require users alter their imports.
The reason for this is largely due to tooling, though the less verbose API certainly is convenient. With this setup our tooling responsibilities diminish greatly. A single entry point and a single bundle is what Microbundle does best. I'd also like to argue that named exports are far less ambiguous in terms of what they do vs. our current environment.
If I were to use
import ColorPicker from 'react-colorful';
then I'd have no idea what typeColorPicker
was without looking at the documentation to see thatreact-colorful
's default export was the Hex color picker. Named exports cut right through this, while still offering options.import { HexColorPicker } from 'react-colorful';
tells me exactly what kind of color picker I have, and I can use{ HexColorPicker as ColorPicker }
if I so desired to keep the rest of my code the exact same.In my mind, ideally, a CSS import route would look like:
import 'react-colorful/css/index.css';
. Not yet implemented in any way, but I do like the idea of moving the CSS outside ofdist/
. Makes for a cleaner path IMO.import 'react-colorful/dist/index.css';
just seems like it isn't as nice.Internal Changes (and why)
build-package.js
With a single top-level entry point our app will be easier to manage. All library exports (excluding css) will be in a single file, period. It reduces complexity as no longer will we have 7 separate files that decide what our output will look like. This is much simpler to deal with.
Our top-level for the library looks quite messy right now, solely due to the types targeting issues outlined here: Typescript: Seemingly no way to target file lower down the tree? developit/microbundle#710 (comment). However, with a top-level entry point like
index.ts
, we can easily target this for our"types"
field resolving the issue. With that, I suggest we revert back to the original structure where the packages are nested away like the following:This structure was much simpler and cleaner.
The removal of
build-packages.js
comes as it is no longer needed. With a single entry point and named exports, Microbundle can handle the task easily on its own. This creates around a 4-5x increase in bundling speed. Obviously this isn't a magical speed increase, but due to the library changes. Still worth noting, faster build times == nicer experience, which is always a good thing.