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: make disable default style env variable work for client side bundler and nextjs #787

Merged
merged 2 commits into from
Mar 2, 2023

Conversation

Jokcy
Copy link
Contributor

@Jokcy Jokcy commented Mar 1, 2023

What kind of change does this pull request introduce?

Make SANDPACK_BARE_COMPONENTS work for client bundler, and add Nextjs support.

close #713

What is the current behavior?

Not work for nextjs

What is the new behavior?

work for nextjs

Checklist

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

@vercel
Copy link

vercel bot commented Mar 1, 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 1, 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 c95a960:

Sandbox Source
Sandpack Configuration

@vercel
Copy link

vercel bot commented Mar 1, 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 1, 2023 at 2:26PM (UTC)

@danilowoz danilowoz changed the title make disable default style env variable work for client side bundler and nextjs fix: make disable default style env variable work for client side bundler and nextjs Mar 1, 2023
@danilowoz
Copy link
Member

danilowoz commented Mar 1, 2023

Hey, @Jokcy! Thanks for opening this PR. But I didn't get why we need to make these changes to work on Nextjs
I just created a nextjs sandbox that works as expected on server/client: https://codesandbox.io/p/sandbox/infallible-ardinghelli-ybxh7

Could you highlight what's the current issue and how it solves it?

Edit: just saw your another comment on the issue, but I still can't understand what's the difference

@danilowoz
Copy link
Member

@Jokcy I updated your sandbox using another approach and it worked as expected, check the next.config.js file
https://codesandbox.io/p/sandbox/restless-sun-3kzd1q

@Jokcy
Copy link
Contributor Author

Jokcy commented Mar 1, 2023

image

Can not open it, and why I try to sign in with Github it redirect to localhost:4000

@Jokcy
Copy link
Contributor Author

Jokcy commented Mar 1, 2023

It will work if you use env in next.config.js because env variables provided by this way will both take effect on server/client side. This did solve this problem but it's not the recommended way to provide env variables, it was hard coded into config file and diffcult to make a change in different environment (local/test/production etc). https://nextjs.org/docs/basic-features/environment-variables this is the recommended way. But for this case in nextjs it's enough for me.

Also I'm not sure if the typeof process !== 'undefined' will work correctly in other build tools (in my experience it should always be false for client side bundler), I will try vite/webpack later.

@danilowoz
Copy link
Member

CodeSandbox is down now, we're taking a look what's going on. Thanks

@Jokcy
Copy link
Contributor Author

Jokcy commented Mar 1, 2023

@danilowoz I tried with vite, define variable in vite.config.js

export default defineConfig({
    plugins: [react()],
    define: {
        "process.env.SANDPACK_BARE_COMPONENTS": "true",
    },
});

It worked at dev mode because on dev mode vite will actually add an process object, but when you bundle the code for production, vite will directly replace the process.env.xxx and will not expose a process object.

image

So not matter if we add NEXT_PUBLIC_SANDPACK_BARE_COMPONENTS, we should still remove the typeof process !== 'undefined' condition。

@danilowoz
Copy link
Member

Got it! It makes sense. Could you please update the documentation with the new env var you introduced? And then, let's get this merged

@Jokcy
Copy link
Contributor Author

Jokcy commented Mar 2, 2023

@danilowoz done!

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

@danilowoz danilowoz merged commit 2a48214 into codesandbox:main Mar 2, 2023
@origami-z
Copy link
Contributor

This breaks on vite react now with process is not defined

image

https://codesandbox.io/p/sandbox/vite-react-sandpack-tqef1u?file=%2FREADME.md

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.

Support remove default style on client side
3 participants