Skip to content
This repository was archived by the owner on Jul 19, 2019. It is now read-only.

Conversation

@dbunker
Copy link
Contributor

@dbunker dbunker commented Jan 18, 2015

No description provided.

@zpao
Copy link
Member

zpao commented Jan 22, 2015

Is Flask super common? My original goal here was that the servers would not require any dependencies be installed. I didn't want anybody to really have to think about the server part, it should "just work" so the reader can focus on React and not have to worry. I didn't get there with the node server (mostly because of lack of time and the fact that npm is pretty much required), though I've considered going back to make sure it does.

The server we have works and there's no obvious advantage to using Flask besides that the code is (much) cleaner. But I wouldnt mind being convinced.

@dbunker
Copy link
Contributor Author

dbunker commented Jan 23, 2015

It's sort of arbitrary what gets included in the standard library of a programming language. C, for example, doesn't include any http server, so unless you want to implement HTTP starting from scratch you're not going to be getting very far. I doubt many people are writing server side code using SimpleHTTPServer, an awful lot are writing code using Flask though, it's quite popular, at least by stargazer count.

I'd say the main purpose of server side code in a tutorial for client side code is so the reader can immediately understand what the server is doing and then immediately return to the tutorial at hand. I can't say I immediately understood what the SimpleHTTPServer code was doing. Since we're assuming the user will go to whichever they know the best, ruby, node, or python, it's reasonable to assume if they go to python they know what pip is, or at least could google it sufficiently to install Flask. Those who know Flask would quickly know what the code does, for the rest, the code seems clearer, at least to me, than using SimpleHTTPServer.

Also updated the code to reflect that server code writes to _comments.txt. Added pretty printing, though could remove that to match up to the other servers.

@sophiebits
Copy link
Member

Flask is probably about as popular as Sinatra. (And as someone who writes^Wused to write Python on a daily basis, Flask is much more familar to me than SimpleHTTPRequestHandler.)

server.py Outdated
Copy link

Choose a reason for hiding this comment

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

Why remove the __name__ check? Putting code in the root context like that is a bad practise.

Copy link
Member

Choose a reason for hiding this comment

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

(+1)

@zpao
Copy link
Member

zpao commented Feb 2, 2015

Ok, I'm game then. Does pip have anything to describe dependencies, like package.json for npm and Gemfile for ruby?

@zpao zpao mentioned this pull request Feb 2, 2015
@zpao
Copy link
Member

zpao commented Feb 2, 2015

See also, #20, where we're going to make it so the json is reread before every response. We should make sure that functionality doesn't get lost in the switch.

Also, before we can merge this, we'll need the CLA signed (https://code.facebook.com/cla). Our bot wasn't set up right and missed PRs in this repo.

Also also, @spicyj should we switch to using Sinatra for the ruby version? It's one more place where something could go wrong. (it might actually be better to keep 2 versions, including for this... server_simple.py and server_flask.py, thoughts?)

@sophiebits
Copy link
Member

I don't really want more versions. One per language already feels like a lot to me.

@zopieux
Copy link

zopieux commented Feb 2, 2015

@zpao

Does pip have anything to describe dependencies, like package.json for npm and Gemfile for ruby?

Use a requirements.txt file with the single line flask. I would also suggest to recommand using a virtualenv.

@dbunker
Copy link
Contributor Author

dbunker commented Feb 2, 2015

Adding a requirements.txt might be overkill for something this simple, pip install flask and gem install sinatra could be enough. But would be happy to add it if we want it to match up to the node server's use of npm and package.json.

@sophiebits
Copy link
Member

Yes, let's have a requirements.txt. Ideally with a version number too.

@zopieux
Copy link

zopieux commented Feb 2, 2015

I don't see the point of freezing the flask version given the simplicity of the example code. Remember you prevent users from getting useful updates (incl. performance and security) when freezing.

@sophiebits
Copy link
Member

You also prevent your code from breaking if Flask makes breaking API changes. I agree it doesn't make a big difference for a small script.

@zpao zpao merged commit 8e78f6c into reactjs:master Feb 6, 2015
zpao added a commit that referenced this pull request Feb 6, 2015
shenxl pushed a commit to shenxl/react-tutorial that referenced this pull request Sep 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants