jotaijs / jotai-devtools

A powerful toolkit to enhance your development experience with Jotai
https://jotai.org/docs/tools/devtools
MIT License
134 stars 33 forks source link

refactor: replace @tabler/icons with @tabler/icons-react for smaller bundle sizes #44

Closed blanky0230 closed 1 year ago

blanky0230 commented 1 year ago

Motivation

We were previously using jotai-devtools in version 0.2.0 after seeing how great the ui looks with the 0.3.0 version we were excited to upgrade!

We however found, that the bundle size has increased significantly!

To reproduce I had created a simple example using create-react-app with Typescript:

App.tsx:

import React from 'react';
import './App.css';
import { DevTools } from 'jotai-devtools';
import { Component } from './Component';
import { createStore, Provider } from 'jotai';
const store = createStore();
function App() {
  return (
    <div className="App">
      <Provider store={store}>
        <DevTools />
        <Component />
      </Provider>
    </div>
  );
}

export default App;

Component.tsx:

import React from "react";
import { atom, useAtom } from "jotai";

const countAtom = atom(0);
countAtom.debugLabel = "countAtom";

export function Component() {

    const [count, setCount] = useAtom(countAtom);

    return (<div>
        {count}
        <button onClick={() => setCount(count + 1)}>+</button>
    </div>)

}

Now, when Using jotai-devtools in version 0.3.1 analyzing the production bundle of the CRA-App with source-map-explorer yields: tabler_icons

So I thought I'd look into @tabler/icons and found that they recently published a package: @tabler/icons-react promising: "It's build with ESmodules so it's completely tree-shakable. Each icon can be imported as a component."

So I thought I'd give it a shot to replace the @tabler/icons with @tabler/icons-react and see how that looks: tabler_icons_react

I think that looks like a good downsizing by roughly 700KB. I am not an expert on bundlers and optimizing for bundle sizes, I just thought this could be an easy improvement, so let me know what you think :)

codesandbox-ci[bot] commented 1 year ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c1c3c8c3242f93dae036ff88077422751011626d:

Sandbox Source
React Configuration
React Typescript Configuration
arjunvegda commented 1 year ago

First off, this is great! Thanks for the investigation and for creating a PR 🙌

<DevTools/> component is designed to be tree-shaken by your bundler (in this case CRA) in a production build. So we should not be seeing it in the bundle at all 🤔

My theory is that CRA imports jotai-devtools's ESM module that wraps this check with (import.meta.env && import.meta.env.MODE) !== "production". AFAIK, CRA currently does not support import.meta, hence, the check is truthy, and it includes <DevTools/> in your production build.

I suppose our options are

  1. jotai-devtools adopts process.env.NODE_ENV for ESM modules (I'd want to avoid this)
  2. Have CRA support import.meta (Highly unlikely as CRA is vanishing as the industry moves towards adopting SSR/Next.js based client-side apps)
  3. CRA users switch to one of the React frameworks
  4. CRA users somehow use the babel-plugin-syntax-import-meta plugin (may need to eject 🤔)
  5. Wrap <DevTools/> with a process.env.NODE_ENV check on your end like so const DevOnlyJotaiDevTools = () => process.env.NODE_ENV !== 'production' ? <DevTools/> : null;
  6. Import DevTools using CJS sytanx i.e. use require to import it. Eg. const { DevTools } = require('jotai-devtools')

I'd still like to merge this change though (less bundle size is always a win 😁).

blanky0230 commented 1 year ago

component is designed to be tree-shaken by your bundler (in this case CRA) in a production build. So we should not be seeing it in the bundle at all

My theory is that CRA imports jotai-devtools's ESM module that wraps this check with (import.meta.env && import.meta.env.MODE) !== "production". AFAIK, CRA currently does not support import.meta, hence, the check is truthy, and it includes in your production build.

Yeah that's also something that surprised me too. Maybe that's due to how the Module is structured such that the MantineTheme and DevToolsMain will always be evaluated.

It seems to me that the simple if (!__DEV__) return null does it's job to hide to hide away the devtools in production bundle, still uses a bunch of libraries to evaluate the code until that point.

Maybe there's also ways we can split things up there a bit and leverage (default) exports more to reduce the amount of imported code in production bundles.

As for now it seems that at DevTools.tsx there are dependencies being imported and evaluated, before we even get down to the Component where check whether or not we are in Dev mode.

Could you help test it on Vite and Next as well?

I will try setting up lab-projects this weekend and report my findings.

Also, how did we verify the bundle size drop? For some reason, CodeSandbox CI shows the same size thinking

The library should stay the same size (after all, we didn't really change anything) - what should minimize the the prod bundle sizes of the Applications USING Jotai-Devtools.

To measure that in my CRA-Example I used source-map-explorer. So in the CRA I ran that against the /dist folder after running npm run build .

arjunvegda commented 1 year ago

It seems to me that the simple if (!DEV) return null does it's job to hide to hide away the devtools in production bundle, still uses a bunch of libraries to evaluate the code until that point.

Do you mean that the <DevTools/> is still being bundled despite adding an env check? Evaluation should only occur during compile time 🤔 and the component and the libraries (Mantine) used by it should not be included in your bundle. Which option did we go with from the above?

Re: Testing on Next js I tested this PR on Next JS (CSB here), and its failing with this error, have you ran into this before or know what could be up 🤔?

image
sari-shipt commented 1 year ago

It seems to me that the simple if (!__DEV__) return null does it's job to hide to hide away the devtools in production bundle, still uses a bunch of libraries to evaluate the code until that point.

We are facing this issue as well, @blanky0230 / @arjunvegda . Any luck figuring out how to avoid that?

arjunvegda commented 1 year ago

It seems to me that the simple if (!__DEV__) return null does it's job to hide to hide away the devtools in production bundle, still uses a bunch of libraries to evaluate the code until that point.

We are facing this issue as well, @blanky0230 / @arjunvegda . Any luck figuring out how to avoid that?

Sorry, I'm unsure about the issue we're referring to here ^. Could you elaborate a little on it? It might be easier to file a discussion

If the <DevTools/> is being included in the bundle, one of the solutions in this comment should help

sari-shipt commented 1 year ago

Sorry, I'm unsure about the issue we're referring to here ^. Could you elaborate a little on it? It might be easier to file a discussion

If the <DevTools/> is being included in the bundle, one of the solutions in this comment should help

@arjunvegda - The issue is that DevTools is being included in the bundle. We tried all suggested solutions and, unfortunately, none of them worked.

arjunvegda commented 1 year ago

Sorry, I'm unsure about the issue we're referring to here ^. Could you elaborate a little on it? It might be easier to file a discussion If the <DevTools/> is being included in the bundle, one of the solutions in this comment should help

@arjunvegda - The issue is that DevTools is being included in the bundle. We tried all suggested solutions and, unfortunately, none of them worked.

That's interesting! In that case could you please file a separate discussion and provide us with a small repro? I can look into it.

sari-shipt commented 1 year ago

That's interesting! In that case could you please file a separate discussion and provide us with a small repro? I can look into it.

hmmm this is interesting. Whenever I created a quick NextJS/React repo the prod build size went from 74 kb to 74.2 kb, while my actual repo went from 596kb to 1.73kb just by importing DevTools. I can't share the company repo, unfortunately, but any idea what could be causing it? I literally only imported DevTools <DevTools store={jotaiState} />

image image
sari-shipt commented 1 year ago

That's interesting! In that case could you please file a separate discussion and provide us with a small repro? I can look into it.

hmmm this is interesting. Whenever I created a quick NextJS/React repo the prod build size went from 74 kb to 74.2 kb, while my actual repo went from 596kb to 1.73kb just by importing DevTools. I can't share the company repo, unfortunately, but any idea what could be causing it? I literally only imported DevTools <DevTools store={jotaiState} />

Similar issue @blanky0230 ??

Thoughts @arjunvegda ?

arjunvegda commented 1 year ago

That's interesting! In that case could you please file a separate discussion and provide us with a small repro? I can look into it.

hmmm this is interesting. Whenever I created a quick NextJS/React repo the prod build size went from 74 kb to 74.2 kb, while my actual repo went from 596kb to 1.73kb just by importing DevTools. I can't share the company repo, unfortunately, but any idea what could be causing it? I literally only imported DevTools <DevTools store={jotaiState} />

Similar issue @blanky0230 ??

Thoughts @arjunvegda ?

Let's avoid using PRs for discussions, please continue the conversation here

blanky0230 commented 1 year ago

It seems to me that the simple if (!DEV) return null does it's job to hide to hide away the devtools in production bundle, still uses a bunch of libraries to evaluate the code until that point.

Do you mean that the <DevTools/> is still being bundled despite adding an env check? Evaluation should only occur during compile time thinking and the component and the libraries (Mantine) used by it should not be included in your bundle. Which option did we go with from the above?

Re: Testing on Next js I tested this PR on Next JS (CSB here), and its failing with this error, have you ran into this before or know what could be up thinking?

image

I don't know what CSB does there tbho...

For local testing I checked out the repo and installed jotai-devtools by filepath.

I tested the setup now with Vite and Next, where I get similar results for bundling. I will attach Screenshots if needed.

arjunvegda commented 1 year ago

It seems to me that the simple if (!DEV) return null does it's job to hide to hide away the devtools in production bundle, still uses a bunch of libraries to evaluate the code until that point.

Do you mean that the <DevTools/> is still being bundled despite adding an env check? Evaluation should only occur during compile time thinking and the component and the libraries (Mantine) used by it should not be included in your bundle. Which option did we go with from the above? Re: Testing on Next js I tested this PR on Next JS (CSB here), and its failing with this error, have you ran into this before or know what could be up thinking?

image

I don't know what CSB does there tbho...

For local testing I checked out the repo and installed jotai-devtools by filepath.

I tested the setup now with Vite and Next, where I get similar results for bundling. I will attach Screenshots if needed.

So it's working fine locally? Could you try updating @tabler/icons-react to 2.16.0 (latest), see if that fixes the issue on CSB?

arjunvegda commented 1 year ago

Fixed in #64