fix: building components with rollup (BREAKING CHANGE) #536
Conversation
masone
changed the title
autoricardo-bot
deployed
to
branch-carforyou-components-pkg-esm-rollup
autoricardo-bot
deployed
to
branch-carforyou-components-pkg-esm-rollup
autoricardo-bot
deployed
to
branch-carforyou-components-pkg-esm-rollup
autoricardo-bot
deployed
to
branch-carforyou-components-pkg-esm-rollup
masone
requested review from
a team,
GutierrezGo,
ufkaya and
Marine-Berthier
and removed request for
a team
autoricardo-bot
deployed
to
branch-carforyou-components-pkg-esm-rollup
| "path": "pkg/dist-web/index.js", | ||
| "maxSize": "25 kB" | ||
| "path": "pkg/cjs/index.js", | ||
| "maxSize": "30 kB" |
There was a problem hiding this comment.
Why does it make the module bigger? Do we have node only deps?
There was a problem hiding this comment.
the check was off - dist-web was building separate files since #459 and the check was only focussing on the entrypoint instead of the full bundle
autoricardo-bot
deployed
to
branch-carforyou-components-pkg-esm-rollup
| stories: ['../src/**/*.stories.@(js|tsx)'], | ||
| stories: ['./stories/**/*.stories.@(js|tsx)'], |
There was a problem hiding this comment.
Why are we piggybacking on collocating stories with components? We've put an effort into it to have it well grouped and improve developer experience in that regard. Interested to hear your motivation here.
There was a problem hiding this comment.
Oh nvm, they weren't truly collocated, but in src/stories instead...
There was a problem hiding this comment.
yes, I had the exactly same thought process - moving them all to src was not was we intended. since storybook and the build need different configs sometimes, I ended up separating completely
There was a problem hiding this comment.
Do we still want to properly co-locate them? What is your view on it?
There was a problem hiding this comment.
I see little benefit by now to be honest...
| @@ -2,8 +2,8 @@ import React, { FC } from "react" | |||
|
|
|||
| import { StoryProps } from "./storyProps" | |||
| import StoryContainer from "./storyContainer" | |||
| import AlertMessage, { AlertMessageProps } from "../components/alertMessage" | |||
| import MailSent from "../../.storybook/icons/mailSent" | |||
| import AlertMessage, { AlertMessageProps } from "../../src/components/alertMessage" | |||
There was a problem hiding this comment.
Food for thought - if we import from the index we'd have a bit less pain moving those around. Plus it would fail if the component isn't exported from the package.
There was a problem hiding this comment.
very good point - I'll move that to another effort though
| commonjs(), | ||
| typescript({ | ||
| tsconfig: "./tsconfig.json", | ||
| exclude: ["**/__*__/**/*"], |
There was a problem hiding this comment.
Don't we need to exclude stories too?
There was a problem hiding this comment.
not now that they're in .storybook - that's one of the simplifications I was able to make with the move
| import { DummyTooltip } from "../../../stories/badges/dummyTooltip" | ||
| import { DummyTooltip } from "../../../../.storybook/stories/badges/dummyTooltip" |
There was a problem hiding this comment.
I would've not expected to import from stories in tests, looks very weird.
Can we extract those dummy components somewhere where they are properly shared?
(not necessarily now, but in a follow-up tackled in due time)
There was a problem hiding this comment.
absolutely - had the same reaction. I already touched so many things in this PR and would like to push that to a separate effort
autoricardo-bot
deployed
to
branch-carforyou-components-pkg-esm-rollup
autoricardo-bot
temporarily deployed
to
branch-carforyou-components-pkg-esm-rollup
|
🎉 This PR is included in version 22.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Todo:
References carforyou/carforyou-docs#147
Moving away from pika/pack, following the decisions outlined in the RFC above. Zero dependencies for a build