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

separate components in different files #188

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Conversation

caub
Copy link

@caub caub commented Apr 20, 2022

No description provided.

@caub
Copy link
Author

caub commented Apr 20, 2022

To facilitate dynamic import and tree shaking

@caub
Copy link
Author

caub commented Apr 20, 2022

Build is not fully succeeding, would need a bit of help please

Copy link
Owner

@zpao zpao left a 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.

yarn.lock Outdated Show resolved Hide resolved
src/index.tsx Show resolved Hide resolved
@caub
Copy link
Author

caub commented Apr 21, 2022

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'));

@zpao
Copy link
Owner

zpao commented Apr 27, 2022

Ah right, React.lazy. I guess that's probably a good reason. I really don't love that this is limited to default exports though. Seems like they may expand it in the future.

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.

@kachkaev
Copy link

Here is a new reason to merge something like this:

@@ -0,0 +1,158 @@
/**

Choose a reason for hiding this comment

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

Suggested change
/**
"use client"
/**

* SPDX-License-Identifier: ISC
*/

import React, {useRef, useEffect} from 'react';
Copy link

@kachkaev kachkaev May 27, 2024

Choose a reason for hiding this comment

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

Suggested change
import React, {useRef, useEffect} from 'react';
import * as React from 'react';
import {useRef, useEffect} from 'react';

or

Suggested change
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.

@kachkaev kachkaev mentioned this pull request May 27, 2024
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.

None yet

3 participants