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
@uppy/core: move Uppy
class to its own module
#3225
Conversation
Is there no other way to keep the import |
Users don't have to use the full path, it's only for those who need/want to opt-out of importing |
Everyone wants to opt-out of importing |
If we remove let UIPlugin;
Reflect.defineProperty(module.exports, 'UIPlugin', {
enumerable: true,
configurable: true,
get() {
return UIPlugin ??= require('./UIPlugin')
},
set(value) {
UIPlugin = value
}
}) IMHO the change in this PR is a lesser evil, unless we can think of another solution? |
Thanks for explaining. I'm not knowledgeable on bundlers enough to know if there is another solution. I'm sure @goto-bus-stop would know if there is, but they are on holidays. It's just unfortunate we did a big 2.0 launch blog post with the selling point of not bundling Preact if you import Perhaps good to let this sit till @arturi and @goto-bus-stop are back and then make a decision? Or is this really the only feasible way forward? |
I think we should land this nonetheless, as isolating classes in their own module is good practice anyway, and that doesn't mean we can't find a better solution in the future. Holding on this PR doesn't do our users any good, landing it doesn't make the core problem go away but at least provides a workaround. |
Fair enough! It would be good to add notes about these imports to the blog post and uppy core docs in 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.
The migration guide should also be updated. And perhaps a note at the first code snippet here as well?
Co-authored-by: Merlijn Vos <merlijn@soverin.net>
Yeah, it is unfortunate, but I guess that’s the way to go for now. At least with this PR we have that option to offer — Preact won’t be bundled via |
This would allow users to use Uppy without loading
preact
if they are not using anyUIPlugin
.Fixes: #3220