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

Avoid importing the whole of react-icons (if treeshaking is broken) #3046

Merged
merged 15 commits into from
Sep 1, 2022

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Aug 25, 2022

Reasons for making this change

Due to the bootstrap-4 package, my bundles end up with large chunks of react-icons package, often being the largest import:

Screen Shot 11

This is a common issue with react-icons and apparently the solution is avoid the main package and use @react-icons/all-files instead:

The main package is set up for tree-shaking but it's not working for me, even if my bundler correctly tree-shakes lodash-es for example

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npm run test:update to update snapshots, if needed.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@heath-freenome
Copy link
Member

@fregante FYI, the v4 branch is locked and soon the rjsf-v5 branch will be landing in master. If you want a head-start on the changes, you could rebase this PR on-top-of that branch, or wait for it to be merged to master before continuing

@fregante fregante changed the base branch from master to rjsf-v5 August 25, 2022 17:12
@fregante fregante changed the base branch from rjsf-v5 to master August 25, 2022 17:12
@heath-freenome
Copy link
Member

@fregante master now contains the beta release, feel free to rebase and finish your PR... Do add tests please

@fregante
Copy link
Contributor Author

Tests for what? This only affects builds. To test this one would have to set up a webpack build and set a size limit.

@heath-freenome
Copy link
Member

Tests for what? This only affects builds. To test this one would have to set up a webpack build and set a size limit.

ah, right... well as long as the build still runs then you are ok. You will need to rebase and resolve conflicts

@fregante
Copy link
Contributor Author

It should be good to go now, but the lockfile has conflicts again, so maybe it's best to resolve them and merge the PR when you're ready, otherwise I have to resolve them daily.

Also the postinstall script (lerna bootstrap) is regularly failing for me on Codespaces.

@fregante fregante marked this pull request as ready for review August 31, 2022 02:24
@heath-freenome
Copy link
Member

@fregante I can't update your PR since it is on your fork... I can however upload the package-lock.json I get when I did the npm install with your package.json change... Try replacing what you have with this and I'll rerun the build:

package-lock.zip

@heath-freenome
Copy link
Member

Also the postinstall script (lerna bootstrap) is regularly failing for me on Codespaces.

Are you running node 16? I do know that lerna bootstrap can take a long time and once crashed my computer.

@fregante
Copy link
Contributor Author

@fregante I can't update your PR since it is on your fork..

You should be able to push any changes:
Screen Shot

This is the issue I get:


@fregante ➜ /workspaces/react-jsonschema-form (patch-1) $ npm install
npm WARN deprecated [email protected]: See https://github.com/lydell/source-map-resolve#deprecated

> [email protected] postinstall
> lerna bootstrap

lerna notice cli v5.4.3
lerna info Bootstrapping 11 packages
lerna info Installing external dependencies
lerna ERR! npm install exited undefined in '@rjsf/playground'
lerna ERR! npm install exited undefined in '@rjsf/playground'
lerna WARN complete Waiting for 3 child processes to exit. CTRL-C to exit immediately.
npm ERR! code 1
npm ERR! path /workspaces/react-jsonschema-form
npm ERR! command failed
npm ERR! command sh -c lerna bootstrap

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/codespace/.npm/_logs/2022-08-31T02_58_01_174Z-debug.log

I tried again by checking out the lockfile from master and running npm install again, even if the postinstall failed. Let's see

@fregante
Copy link
Contributor Author

Are you running node 16?

@fregante  /workspaces/react-jsonschema-form (patch-1) $ npm --version
node --version
7.24.2
@fregante  /workspaces/react-jsonschema-form (patch-1) $ node --version
v16.14.2

@heath-freenome
Copy link
Member

Also, if this passes, can you update the CHANGELOG.md file with a description of your fix and the issue it closes?

CHANGELOG.md Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@fregante
Copy link
Contributor Author

Sorry I cannot make any changes, I even tried it on my computer (macOS) and npm install still fails. Feel free to commit any missing changes and push them to my repo, it's possible.

@heath-freenome
Copy link
Member

I still can't make changes so here are the updated snapshots
snapshots.zip

@fregante
Copy link
Contributor Author

fregante commented Sep 1, 2022

I still can't make changes

You're definitely allowed to considering you just pushed 33f4115 to my repo 😁

https://github.com/fregante/react-jsonschema-form/commits/patch-1

See

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/committing-changes-to-a-pull-request-branch-created-from-a-fork

I still can't make changes so here are the updated snapshots
snapshots.zip

Thank you! Committed

@heath-freenome
Copy link
Member

@fregante Go ahead and merge, unless you'd rather I do it

@fregante
Copy link
Contributor Author

fregante commented Sep 1, 2022

Great!

But only collaborators can merge, I don’t have a merge button:

EF1D2578-7AA3-4A0D-A671-567B9FFE159E

@heath-freenome heath-freenome merged commit 7f634c2 into rjsf-team:master Sep 1, 2022
@heath-freenome
Copy link
Member

done!

@fregante fregante deleted the patch-1 branch September 1, 2022 17:07
@fregante
Copy link
Contributor Author

fregante commented Sep 1, 2022

🙌

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.

2 participants