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: simplify local start - remove pnpm from preview script, make it work on Windows and not fail due to bindings #106

Closed

Conversation

wonderwhy-er
Copy link
Collaborator

@wonderwhy-er wonderwhy-er commented Oct 28, 2024

This is a small 2-line change to cleanup run scripts to work better on Windows

Want to make a video on youtube of how to run Bolt.New on windows locally in the simplest way, 2 things complicate it unnecessarily. And I fix them here, its just 2 lines.

  1. I removed pnpm from the preview as other commands use npm anyway and it removes one additional thing for beginners to install and use. npm is good enough for general repo.
  2. I added a fallback for Windows not to use bindings. They are not used currently for local running always.

This should not break Docker part but I don't use docker so would be nice if someone who does checks

@wonderwhy-er wonderwhy-er changed the title Try to fix start for windows Fix scripts to remove pnpm and remove failure to run on windows due to bindings Oct 28, 2024
@wonderwhy-er wonderwhy-er changed the title Fix scripts to remove pnpm and remove failure to run on windows due to bindings Simplify local start: remove pnpm from preview script, make it work on Windows and not fail due to bindings Oct 28, 2024
@wonderwhy-er wonderwhy-er changed the title Simplify local start: remove pnpm from preview script, make it work on Windows and not fail due to bindings build: Simplify local start - remove pnpm from preview script, make it work on Windows and not fail due to bindings Oct 28, 2024
@wonderwhy-er wonderwhy-er changed the title build: Simplify local start - remove pnpm from preview script, make it work on Windows and not fail due to bindings build: simplify local start - remove pnpm from preview script, make it work on Windows and not fail due to bindings Oct 28, 2024
@wonderwhy-er wonderwhy-er changed the title build: simplify local start - remove pnpm from preview script, make it work on Windows and not fail due to bindings fix: simplify local start - remove pnpm from preview script, make it work on Windows and not fail due to bindings Oct 28, 2024
@felipe2g
Copy link

felipe2g commented Oct 29, 2024

its a big decision remove pnpm, if community approve we need remove the pnpm.lock file and include npm

another thing we have to consider is, officia bolt use then...

@wonderwhy-er
Copy link
Collaborator Author

its a big decision remove pnpm, if community approve we need remove the pnpm.lock file and include npm

another thing we have to consider is, officia bolt use then...

Hm, valid point about pnpm.lock and keep in sync with the official repo.
I 100% agree with you that trying to stay closer so we can merge with changes in it in the future is something I would like to do for now.

But build and start commands use npm. While preview does not and it calls build and start commands.
I think it's still valid to keep those things in sync, why require pnpm only for preview?

The reason this came up for me is that I was trying to show people how to install it locally and it required an additional step for them to add pnpm with no real benefit for them.

That is just my thoughts. I can accept the desire to remove pnpm change line.

@wonderwhy-er
Copy link
Collaborator Author

Pushed in a fix, just was testing installation on old Windows latop script I had before that did not work so well, changed to 3 scripts
start:windows
start:UNIX

and start now uses node script to decide which one to run

Tested on Mac and Windows 10

@syndicate604
Copy link

PNPM doesn't work well on Ubuntu even in a venv, so on linux I just use npm and it works fine.
Also chrome canary doesn't work with ubuntu either.

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.

3 participants