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

fix: add new build for styleless #798

Merged
merged 8 commits into from
Mar 14, 2023
Merged

Conversation

Jokcy
Copy link
Contributor

@Jokcy Jokcy commented Mar 4, 2023

What kind of change does this pull request introduce?

We used try to remove the typeof process condition for default styles, but it bring a new issue for vite and a new PR #795 add the condition back.

We need to understand that this kind of assert will always be undefinde for most of bundlers, vite/webpack etc, because there is no process in browser and all of them chose to directly replace process.env.xxx to the specify value, this means:

const a = process.env.xxx // source code

// after compile

const a = true // the value you specified on config

So without remove the typeof process condition the disable default style feature will never work on production (for some reason most of bundler actually provide a process object on dev mode).

It's not recommeded for client module to rely on procee.env.xxx env variables because it's not standard. Different bundle tools handle env with different ways, for example in vite the default supported way to get env is through import.meta.env.xxx, so there is no process.env unless you config it at vite.config.js.

What is the current behavior?

Disable default style will never work on production build.

What is the new behavior?

Add a new build to remove the default style, the remove action happend at build time which we can control. And we can do more with this to remove all the css({...}) code in this build to even reduce the final bundle size.

What steps did you take to test this? This is required before we can merge, make sure to test the flow you've updated.

Build with stitches
image

Build without stitches
image

Checklist

  • Documentation;
  • Storybook (if applicable);
  • Tests;
  • Ready to be merged;

@vercel
Copy link

vercel bot commented Mar 4, 2023

@Jokcy is attempting to deploy a commit to the CodeSandbox Team on Vercel.

A member of the Team first needs to authorize it.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 4, 2023

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 e0e8739:

Sandbox Source
Sandpack Configuration
icy-bush-g4d5ww Configuration

@danilowoz danilowoz changed the title add new build for styleless fix: add new build for styleless Mar 7, 2023
@danilowoz
Copy link
Member

danilowoz commented Mar 7, 2023

Hey, @Jokcy! I loved this solution!

I pushed another version that relies on a new folder, which contains all components without styling:

import { Sandpack } from "@codesandbox/sandpack-react/unstyled";

What do you think?

@Jokcy
Copy link
Contributor Author

Jokcy commented Mar 8, 2023

@danilowoz That's great! I did want to do the same thing but I'm not sure if you will agree this solution so I just implement it in minimal effect. Great job anyway.

I also want to remove the default css code for the unstyled bundle, for example:

// in Stack.tsx

export const stackClassName = css({
  display: "flex",
  flexDirection: "column",
  width: "100%",
  position: "relative",
  backgroundColor: "$colors$surface1",
  gap: 1, // border between components

  [`&:has(.${THEME_PREFIX}-stack)`]: {
    backgroundColor: "$colors$surface2",
  },
});

These code is useless for unstyled bundle, I want to figure out a easy way to remove all these code without a lot of change of our codebase.

Do you want this being include in this PR or another PR should be created?

@danilowoz
Copy link
Member

Thanks! Yeah, let's try to remove these styles on this PR, feel free to give it a try.

@Jokcy
Copy link
Contributor Author

Jokcy commented Mar 10, 2023

@danilowoz I added an rollup plugin to remove the css code for unstyled bundle, please review it and give some feedback. It did reduce the size after add this plugin:

image

I also added the filesize plugin to help show file size.

Copy link
Member

@danilowoz danilowoz left a comment

Choose a reason for hiding this comment

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

THIS IS GREAT! 🚀

I was wondering if, in the future, we can extract the CSS to static files. What do you think?

@Jokcy
Copy link
Contributor Author

Jokcy commented Mar 11, 2023

I was wondering if, in the future, we can extract the CSS to static files. What do you think?

🤔 You mean like pre compile stitches to css files?

@jeetiss
Copy link
Contributor

jeetiss commented Mar 12, 2023

You mean like pre compile stitches to css files?

sounds like https://vanilla-extract.style/

it has plugins for esbuild and rollup to extract styles, but i'm not sure that it will fit sandpack well.

I struggle with current styles a lot and it should be much easier to work with good old CSS

@vercel
Copy link

vercel bot commented Mar 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
sandpack-docs ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 14, 2023 at 10:46AM (UTC)

@Jokcy
Copy link
Contributor Author

Jokcy commented Mar 12, 2023

@danilowoz If we generate static css from stitches, I guess it may break the theme props usage?

@danilowoz
Copy link
Member

yeah, it's not an easy issue to solve.

BTW, can you update this branch with main?

@Jokcy
Copy link
Contributor Author

Jokcy commented Mar 12, 2023

BTW, can you update this branch with main?

You mean rebase main to this branch?

@Jokcy
Copy link
Contributor Author

Jokcy commented Mar 12, 2023

@danilowoz done.

@danilowoz
Copy link
Member

Hey @Jokcy. It seems that this PR introduces a regression on the Console component. I'm getting the following error only on the documentation (dev-mode works fine), which made me conclude that the bundler has been broken.

I spent quite some time trying to figure out what was going on, but with no success. Maybe you have some idea.
Screenshot 2023-03-12 at 14 09 24

@Jokcy
Copy link
Contributor Author

Jokcy commented Mar 12, 2023

@danilowoz Got it, will check later. Do we have some unit/e2e tests to help us more confident about the changes? The unstyled bundle should pass all the cases.

@danilowoz
Copy link
Member

No, we don't. But I'm not if a unit test could help in this case because it looks a like dependency or a bundler issue

@Jokcy
Copy link
Contributor Author

Jokcy commented Mar 13, 2023

@danilowoz The reason is import Ansi from "ansi-to-react"; result to {__esModule: true, default: ƒ} instead of just get the default Ansi component. It's kind of weird...

@Jokcy
Copy link
Contributor Author

Jokcy commented Mar 13, 2023

@danilowoz Find the reason, it's because we output our es bundle to mjs, which seems being handled differently in typescript, change file to index.js and "import": "./dist/index.js" in packagejson fix this issue. Don't know why though, need to do some research.

From: https://www.typescriptlang.org/docs/handbook/esm-node.html

imports might resolve differently from dependencies in node_modules

Seems it did have some difference for typescript to handle import in an esm file.

The easier way is to output filename like index.esm.js.

@jeetiss
Copy link
Contributor

jeetiss commented Mar 14, 2023

I fulgurate out and this is how webpack works. it doesn't apply export default interop to mjs files, breaking the "ansi-to-react" dependency. this is a modern module resolution algorithm from the nodejs and it is enabled only for mjs files

The easier way is to output filename like index.esm.js.

so a name with js extension will solve the problem.

here is repo with minimal reproduction https://github.com/jeetiss/mjs-commonjs-default

Copy link
Member

@danilowoz danilowoz left a comment

Choose a reason for hiding this comment

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

Thank you @Jokcy! Thank you @jeetiss!

Let's get this merged

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