HarveyD / react-component-library

A project skeleton to get your very own React Component Library up and running using Rollup, Typescript, SASS + Storybook
https://blog.harveydelaney.com/creating-your-own-react-component-library/
MIT License
790 stars 167 forks source link

[BUG] Code splitting not working. #17

Open mnlbox opened 4 years ago

mnlbox commented 4 years ago

I checked the code splitting branch and it seems it's not working. After doing changes and try using the component-library in a test app (created by CRA) I got this error: (I build and re-install the package after add code splitting and run yarn cache clean)

./src/App.tsx
Cannot find module: 'react-component-library/build/TestComponent/TestComponent'. Make sure this package is installed.

You can install this package by running: yarn add react-component-library/build/TestComponent/TestComponent.

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I tried to access TestComponent exactly the same as you told in your article, like this:

import TestComponent from 'react-component-library/build/TestComponent/TestComponent';

Also, this is the folder structure after code-splitting contains folders like node_modules and src and it's not clean enough:

.
├── index.d.ts
├── node_modules
│   └── style-inject
│       └── dist
│           ├── style-inject.es.js
│           └── style-inject.es.js.map
├── src
│   ├── index.js
│   ├── index.js.map
│   ├── TestComponent
│   │   ├── index.js
│   │   ├── index.js.map
│   │   ├── TestComponent.js
│   │   ├── TestComponent.js.map
│   │   ├── TestComponent.scss.js
│   │   └── TestComponent.scss.js.map
│   └── TestComponentNew
│       ├── index.js
│       ├── index.js.map
│       ├── TestComponentNew.js
│       ├── TestComponentNew.js.map
│       ├── TestComponentNew.scss.js
│       └── TestComponentNew.scss.js.map
├── TestComponent
│   ├── index.d.ts
│   ├── TestComponent.d.ts
│   └── TestComponent.types.d.ts
├── TestComponentNew
│   ├── index.d.ts
│   ├── TestComponentNew.d.ts
│   └── TestComponentNew.types.d.ts
├── typography.scss
└── variables.scss

It's better to able import component in code splitting like this:

import TestComponent from 'react-component-library/TestComponent';

Rather than this:

import TestComponent from 'react-component-library/build/TestComponent/TestComponent';
HarveyD commented 4 years ago

Issue was resolved in: https://github.com/HarveyD/react-component-library/issues/14

Please update the branch and try again.

In regards to your second point, the best we can do is:

import TestComponent from 'react-component-library/build/TestComponent';

If we use index.ts files in our components and only target one module format (CJS). We can't easily remove the build directory due as that would involve outputting the compiled code in the top level directory. If you're okay with that, change dir from build to '' and update main to 'index.js'

mnlbox commented 4 years ago

@HarveyD Thanks, it's worked. Just one question: I tried to add a new component exactly the same as TestComponent called TestComponentNew and using code-splitting now. But the bundle size for my test app (a CRA application) not changed when I using just TestComponent or both components. It seems it's put the whole component in the bundle in both cases. Do you have any idea about this?

HarveyD commented 4 years ago

You have to add an entrypoint to the new component in rollup-config.js:

input: ["src/index.ts", "src/TestComponent/index.ts"],
mnlbox commented 4 years ago

@HarveyD I added there. I don't know why but this config didin't work for me because when I add preserveModules: true, my build is like this:

├── index.d.ts
├── node_modules
│   └── style-inject
│       └── dist
│           ├── style-inject.es.js
│           └── style-inject.es.js.map
├── src
│   ├── index.js
│   ├── index.js.map
│   ├── TestComponent
│   │   ├── index.js
│   │   ├── index.js.map
│   │   ├── TestComponent.js
│   │   ├── TestComponent.js.map
│   │   ├── TestComponent.scss.js
│   │   └── TestComponent.scss.js.map
│   └── TestComponentNew
│       ├── index.js
│       ├── index.js.map
│       ├── TestComponentNew.js
│       ├── TestComponentNew.js.map
│       ├── TestComponentNew.scss.js
│       └── TestComponentNew.scss.js.map
├── TestComponent
│   ├── index.d.ts
│   ├── TestComponent.d.ts
│   └── TestComponent.types.d.ts
├── TestComponentNew
│   ├── index.d.ts
│   ├── TestComponentNew.d.ts
│   └── TestComponentNew.types.d.ts
├── typography.scss
└── variables.scss

As you can see I have one node_modules inside this build folder that not clean. Also for example for TestComponent component my types is located in TestComponent folder but js modules located in src/TestComponent. So we can't import module like as you told:

import TestComponent from 'react-component-library/build/TestComponent';

I think something is missing here. :thinking:

HarveyD commented 4 years ago

I've updated the branch. You'll need to rebase on it, or look at what I've changed (removed rollup-plugin-postcss) and make those changes :)

mnlbox commented 4 years ago

@HarveyD I applied all of your changes (remove @rollup/plugin-node-resolve as you did in this commit https://github.com/HarveyD/react-component-library/commit/85418202282510d68b79db96b96ca4aea2057dc3) but my output folder structure still is messy like bellow:

.
├── node_modules
├── src
├── typography.scss
├── variables.scss
└── _virtual

I change this line https://github.com/HarveyD/react-component-library/blob/fbe7fc963bccbb61042d6df0adbceb47dc90aec9/tsconfig.json#L5 to "declarationDir": "build/src" and it's a little cleaner now but still I don't know why I have node_modules and _virtual folder here. :thinking: Any idea?

mnlbox commented 4 years ago

Also when I want to use this component library in a react app based on create-react-app I gave a lot of error like this:

import { Button } from "component-library/build/src/Button"
[at-loader] ./node_modules/component-library/node_modules/@types/lodash/ts3.1/common/function.d.ts:594:10 
    TS2300: Duplicate identifier '__'.

[at-loader] ./node_modules/component-library/node_modules/@types/lodash/ts3.1/common/function.d.ts:595:10 
    TS2300: Duplicate identifier 'Function0'.

[at-loader] ./node_modules/component-library/node_modules/@types/lodash/ts3.1/common/function.d.ts:596:10 
    TS2300: Duplicate identifier 'Function1'.

[at-loader] ./node_modules/component-library/node_modules/@types/react/index.d.ts:3115:13 
    TS2717: Subsequent property declarations must have the same type.  Property 'feTurbulence' must be of type 'SVGProps<SVGFETurbulenceElement>', but here has type 'SVGProps<SVGFETurbulenceElement>'.

[at-loader] ./node_modules/component-library/node_modules/@types/react/index.d.ts:3116:13 
    TS2717: Subsequent property declarations must have the same type.  Property 'filter' must be of type 'SVGProps<SVGFilterElement>', but here has type 'SVGProps<SVGFilterElement>'.

It sees it's about our dependencies types: lodash, react, ... It's also intresting because we didn't use lodash and we are using lodash-es currently :thinking:

mnlbox commented 4 years ago

@HarveyD My issue fixed after install and add this module to rollup config: https://github.com/stevenbenisek/rollup-plugin-auto-external Now rollup didn't create node-modules folder in my output. Maybe you can also check it and it's necessary we can add this to the related branch. :wink:

HarveyD commented 4 years ago

Yeah it seems like code splitting and bundling 3rd party dependencies don't play nicely together. The plugin you mentioned checks all the dependencies and prevents them from being bundled (and the node-modules output in the build folder).

This means you'll have to include these 3rd party libraries in projects using your component library. As such, I recommend including them as peerDependencies. The peerDepsExternal plugin will also prevent them from being bundled.

mnlbox commented 4 years ago

@HarveyD Thanks for your answer. Any way to prevent the creation of _tslib.js in preserveModules mode? I think this is the last issue on code splitting build folder structure :wink:

goldins commented 3 years ago

This is sort of unrelated but I saw that you replaced the postcss plugin with sass as part of the fix for this.

When using the sass plugin with @use ../ SASS directives instead of @import, there are various errors such as Error: Expected digit.

I haven't looked into the cause of this, but it's similar behavior in a project of mine.

To reproduce, update TestComponent.scss to use @use:

@use "../../variables";
@use "../../typography";

.test-component {
  background-color: variables.$harvey-white;
  border: 1px solid variables.$harvey-black;
  padding: 16px;
  width: 360px;
  text-align: center;

  .heading {
    @include typography.heading;
  }

  &.test-component-secondary {
    background-color: variables.$harvey-black;
    color: variables.$harvey-white;
  }
}