-
Notifications
You must be signed in to change notification settings - Fork 335
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
separate components in different files #188
base: trunk
Are you sure you want to change the base?
Conversation
To facilitate dynamic import and tree shaking |
Build is not fully succeeding, would need a bit of help please |
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.
First off, thanks for taking the interest and time!
I had thought about doing this in the most recent version but didn't end seeing a huge amount of value. Tree shaking is obviously tooling dependent but most things I've seen is that the file structure is not the important part here, but actually using named imports is. So by using the named exports instead of the default should actually get you there. I plan to remove that default export in the next major version (it's "deprecated" now) in part to help get people to the happy path. I haven't paid close attention though so if there is something I should read about splitting files being necessary, I'd love to read it.
In terms of helping, I don't have time to dig in. It looks like there's an issue generating the .d.ts
file, probably because there's a split now in the module definition.
Further, it looks like the new files aren't being built (even if you skip the --dts
flag in the build script), so there's work to do there. If you're interested in pursuing this, you'll need to make sure it builds, update the example, etc.
But if there's not a great reason to do this in the first place, then the whole effort may be moot.
For lazy loading, since the main component QRCode is deprecated, you'd have to do something like this const QRCode = lazy(() => import('qrcode.react').then(module => ({ default: module.QRCodeSVG }));
<Suspense fallback={<Spinner />}>
<QRCode />
<Suspense fallback={fallback}> With separate files, we could do: const QRCode = lazy(() => import('qrcode.react/lib/QRCodeSVG')); |
Ah right, I'm not going to rush to support this, especially since there's a (reasonably) well documented workaround. We would also need to go chop up qr-code-generator to get this all to build (which I just tested, and is actually a good thing since it shaves off a bit of code), and play around more with other build things to support multiple files. |
Here is a new reason to merge something like this: |
@@ -0,0 +1,158 @@ | |||
/** |
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.
/** | |
"use client" | |
/** |
* SPDX-License-Identifier: ISC | ||
*/ | ||
|
||
import React, {useRef, useEffect} from 'react'; |
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.
import React, {useRef, useEffect} from 'react'; | |
import * as React from 'react'; | |
import {useRef, useEffect} from 'react'; |
or
import React, {useRef, useEffect} from 'react'; | |
import * as React from 'react'; |
if yu want to use React.useRef
/ React.useEffect
.
Default export from react
might be removed in v20 or a bit later. It’s not tree-shakeable.
No description provided.