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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please add back support for -s option for the sake of CRA users #382

Closed
gaearon opened this issue May 29, 2018 · 4 comments
Closed

Please add back support for -s option for the sake of CRA users #382

gaearon opened this issue May 29, 2018 · 4 comments

Comments

@gaearon
Copy link

gaearon commented May 29, 2018

Hi 馃憢

As you might know, Serve is currently featured in production build instructions of every single app created with CRA. We didn鈥檛 add it to CRA ourselves鈥攚e took a PR from @leo (facebook/create-react-app#1760). It鈥檚 a nice tool indeed! Zeit is doing a lot of important work in the React ecosystem, and at the time I thought it would be a nice gesture to recommend it too.

When we took that PR we assumed that serve is a simple utility that wouldn鈥檛 change in important ways for such a simple (from the user鈥檚 point of view) use case as serving a single static folder for an SPA.

However, this changed with Serve 7. Specifically, the -s option was dropped. This means that every single app made with CRA now prints wrong instructions: #379.

Now I agree that normally it would be our fault for including an instruction in the build output without specifying the version. But again, we didn鈥檛 add serve ourselves to CRA. We took a PR from the Zeit team (facebook/create-react-app#1760). That PR didn鈥檛 include a version number so I thought there was an implicit assumption this part of the Serve API isn鈥檛 going to change.

This is ultimately my fault for relying on something external in the instructions. But the current situation is deeply confusing to our users, and I hope you can help. Quoting #379:

My sentiment exactly, as that's what happened to me on a production environment and it took me at least 30 minutes to realize and fix the problem

There are several possible solutions.

We could take facebook/create-react-app#4532. Note this is neither simple nor exhaustive. It means I would have to drop everything I鈥檓 doing, and release patch releases for every major version of react-dev-utils that contains this text.

This still wouldn鈥檛 be sufficient either because, as you can see in facebook/create-react-app#1760 (which is where the instructions were added for the first time), they used to be a part of react-scripts itself! So there is just no possible way to release this change for people pinned to a specific react-scripts version. In practice it means that all apps older than a certain version will keep printing wrong instructions.

Our philosophy is that people shouldn鈥檛 feel obligated to upgrade their toolchains, and it is sad that their projects will now be confusing to everyone who runs them. This includes both apps and open source examples and demos through which people learn React.

And of course, people who ejected before it was moved to react-dev-utils just have that message hardcoded into their build scripts. There is no way we could fix it.

Even if we take facebook/create-react-app#4532, it seems like it does something different now (without -s the whole point of using serve is lost for us). Using a config file is not an option for us, and is pretty much a showstopper for CRA. This has to be a simple single command.

So this means that we鈥檒l not only have to do all the work above (and it will not be sufficient, as I explained, because many people will still see the old message), but also quickly find a maintained replacement before too many people get confused by their broken build testing instructions.

An alternative solution would be to keep -s. How complex would you estimate this to be?

If you鈥檙e convinced dropping it is best, is there any chance you could give us at least a few weeks to do those patch releases and do our best effort at updating the messages where it is possible? Maybe once we figure out another tool to migrate to, you could make serve -s print a message that also recommends it? At least this way old projects would still print something sensible.

To be clear, I鈥檓 not concerned for myself. This is just very unfortunate for our users and especially beginners who learn not to trust what their tools tell them.

@sjdweb
Copy link

sjdweb commented May 29, 2018

Also, the -p option has been removed. We have switched to using serve@v6 rather than latest in our Dockerfile.

ERROR: Unknown or unexpected option: -p

@leo
Copy link
Contributor

leo commented May 29, 2018

Just jumped on DM with @gaearon and we figured out a great solution. Sorry for the inconvenience we've caused. I will personally ensure that this won't happen again!

The latest release of serve allows for serve -s and serve --single, which both apply the following rewrite preset:

{
  "source": "**",
  "destination": "/index.html"
}

Effectively making CRA apps work just as before again.

For @sjdweb, we also have a solution: Please just pass -l <port> instead of -p <port>.

@gaearon
Copy link
Author

gaearon commented May 29, 2018

Thanks 馃挏馃挏馃挏

@franciscop-invast
Copy link

franciscop-invast commented May 31, 2018

TL;DR: always lock down your dependencies.

For anyone coming here, I'd like to suggest an extra fix for devs who got this problem. We were manually installing serve and running it:

npm install serve
serve -s build -p 3000

Which just started causing the strange error ERROR: Unknown or unexpected option: -s with the code that was running just a day before.

The better way IMHO is to install serve as a dependency, so that the version is somewhat fixed with no breaking changes, and then run it as npm script:

"scripts": {
  "serve": "serve -s build -p 3000"
}

Then npm run serve will work in the same way in dev and prod. You could add the @6 on the npm install serve@6, but then you'd have to make sure the versions are synced manually while I prefer to keep dev and prod versions the same in code.

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

No branches or pull requests

4 participants