Add firebase-based tic-tac-toe sample. #560

jerjou · 2016-10-06T20:46:29Z

Mostly a refactor of stuff from @mogar1980 #558

jerjou · 2016-10-06T20:56:31Z

FYI I still intend to write tests for this, but I figured I wanted some eyes on it first to make sure there're no objections.

theacodes · 2016-10-06T20:57:08Z

appengine/standard/firebase/firetactoe/firetactoe.py

+#
+#     https://console.firebase.google.com/project/_/settings/database
+#
+FIRE_DB_KEY = 'REPLACE_WITH_YOUR_DB_KEY'


use env_variables in app.yaml?

Is that very much better than having it inline here? We'll still need to say "open up a file and fill in that placeholder", and at least here it's more in-context?

theacodes · 2016-10-06T20:58:07Z

appengine/standard/firebase/firetactoe/firetactoe.py

+app = flask.Flask(__name__)
+
+
+def _get_firebase_config():


This is pretty gnarly, is there another way to get the info you need?

Yeah - I agree.

The problem is that I need the info from _FIREBASE_CONFIG_TEMPLATE in two places -

the client needs it 'cuz it receives events from this endpoint

the server needs it because it sends events to this endpoint.

I could inject it into the template, but the way you get it from the firebase console is it gives you an html snippet to copy/paste, and I'd rather say "paste the entire snippet into a file" than "paste the snippet into a file and then look for the databaseURL and copy that value into this placeholder in firetactoe.py".

Alternatively, I could delete this function (it's only used once) and instead of doing config['databaseURL'], I could do:

with open(os.path.join(_CWD, 'templates', _FIREBASE_CONFIG_TEMPLATE)) as f: regex = re.compile(r'\bdatabaseURL\b.*?["']([^'"]+)['"]') match = next(regex.search(line) for line in f if regex.search(line)) databaseURL = match.group(1) # etc...

which isn't as correct, but does the job. It's slightly prettier... okay, updated to use a form of that. LMKWYT

What does this template look like? Can you paste an example?

Fine, regex it is.

theacodes · 2016-10-06T20:58:52Z

appengine/standard/firebase/firetactoe/firetactoe.py

+
+
+def _send_firebase_message(u_id, message=None):
+    global _FIRE_URL


Don't modify globals if you can help it, and definitely don't modify global constants. Can this just be pulled into a function?

Mostly I'm trying to cache the url, to avoid reading a file from disk on every request. I dunno - premature optimization? :-)

That's fine, but can you pull all of this caching stuff into it's own function? _get_fire_url?

theacodes · 2016-10-06T20:59:56Z

appengine/standard/firebase/firetactoe/firetactoe.py

+    }
+    exp = datetime.timedelta(minutes=60)
+    return jwt.generate_jwt(
+        payload, RSA.importKey(credentials['private_key']), 'RS256', exp)


So it seems the only reason we require them to give the private key is so that pyjwt can sign the jwt. However, App engine can actually sign blob directly using app_identity. We'd have to forgo the jwt library, but do you think that's doable?

I'm not too familiar with how jwts work, so I wasn't sure if the app_identity signing was the same as the tokens firebase expects.

I searched the app_identity page for the string 'jwt' and came up blank, so kept this :-/

encoding a jwt is pretty straightforward, just use this code: https://github.com/GoogleCloudPlatform/google-auth-library-python/blob/master/google/auth/jwt.py#L55

Don't worry about the kid

Replace signer.sign with app_identity.sign_blob.

Just noticed that you've done this. 👍

I ran into a problem with this which I fixed, but then realized that using the gcloud way of providing ADC doesn't provide app_identity with everything it needs; so modified the README to use a service account instead.

theacodes · 2016-10-06T21:00:19Z

appengine/standard/firebase/firetactoe/firetactoe.py

+        payload, RSA.importKey(credentials['private_key']), 'RS256', exp)
+
+
+class Wins():


Does this need to be a class, or could it just be some constants?

I figured the class groups them together in a namespace, which is nice.

Not a strong enough argument for class. :)

I'd prefer that over having a slew of instance variables and initialization logic in the global namespace.
What would you suggest?

Just

X_WIN_PATTERNS = ...

It's fine to have these as constants because, well, they are constants. There's no argument for having them in a class. Classes are for collective state or behavior. You could maybe argue for having them in a separate module.

I'm unconvinced, but want to get this in sooner rather than later.
Done.

theacodes · 2016-10-06T21:00:43Z

appengine/standard/firebase/firetactoe/firetactoe.py

+    o_wins = map(lambda s: re.compile(s), o_win_patterns)
+
+
+class Game(db.Model):


ndb instead of db?

theacodes · 2016-10-06T21:01:10Z

appengine/standard/firebase/firetactoe/firetactoe.py

+                    return
+
+
+def login_required(f):


s/f/view_func

theacodes · 2016-10-06T21:02:20Z

appengine/standard/firebase/firetactoe/static/main.js

+   */
+  function updateGame() {
+    for (var i = 0; i < 9; i++) {
+      var square = document.getElementById(i);


jquery could make a lot of this dom manipulation much easier to read.

I considered that too, but I don't think this script is doing anything complex enough to warrant pulling in jquery. The equivalent jquery would have a similar LOC count (I just tried), and then we're importing a relatively heavy-weight library.

(not to say there might not be wins to be gained with a larger refactor... (/me eyes the manual style setting below) but it's non-obvious and the clock is ticking :-/)

It won't decrease the line count, but it'll make things easier. (Also, jquery is hardly heavyweight).

I disagree, but whatever. Refactored.

theacodes · 2016-10-06T21:04:39Z

appengine/standard/firebase/firetactoe/templates/fire_index.html

+    <link rel="stylesheet" type="text/css" href="static/main.css" />
+    <script src="static/main.js"></script>
+    <script>
+      initGame('{{ game_key }}', '{{ me }}', '{{ token }}', '{{ channel_id }}',


I would prefer you to store these as data- attributes on some well-known element (body would be fine).

Any particular reason why that seems better? That feels a little global-variable-y to me, whereas passing in the parameters to init with as arguments seems like the Right Thing to do?

You can still invoke the function:

var $config = $('something), game_key = $config.data('game-key'), ...; initGame(game_key, ...);

It just decouples the JS from the template engine - I really, really abhor mixing server-side template interpolation in javascript.

Why?
I like it - it puts the information precisely where it's needed, and the templating system makes sure it's safe.

jerjou · 2016-10-07T00:41:05Z

Updated to eliminate need for @login_required decorator, as well as separate Firebase api key step

jerjou · 2016-10-07T04:51:09Z

Removed dependency on service accounts, in favor of ADC all the way.

theacodes · 2016-10-07T17:19:30Z

appengine/standard/firebase/firetactoe/firetactoe.py

+
+
+def _send_firebase_message(u_id, message=None, _memo={}):
+    if 'http' not in _memo:


Pull memoization into it's own function, _get_http.

jerjou · 2016-10-07T21:07:50Z

Updated. PTAL

@mogar1980

Mostly a refactor of stuff from @mogar1980

theacodes

Good enough for rock 'n' roll, I guess.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 6, 2016

jerjou assigned theacodes Oct 6, 2016

jerjou force-pushed the firetactoe branch 3 times, most recently from 9503b8d to 175c133 Compare October 6, 2016 20:53

jerjou mentioned this pull request Oct 6, 2016

firetactoe pull request #558

Closed

jerjou force-pushed the firetactoe branch from ca4ee83 to c6e4b4b Compare October 6, 2016 21:02

theacodes suggested changes Oct 6, 2016

View reviewed changes

Fix app engine readme

0932d55

jerjou force-pushed the firetactoe branch 2 times, most recently from d1f55ba to 8fffdf3 Compare October 7, 2016 00:40

theacodes reviewed Oct 7, 2016

View reviewed changes

jerjou force-pushed the firetactoe branch from acfae9a to 2b6420f Compare October 7, 2016 19:56

Jerjou Cheng added 3 commits October 7, 2016 14:17

Add firebase-based tic-tac-toe sample.

4d57d47

Mostly a refactor of stuff from @mogar1980

Use Application Default Credentials.

e0470c9

Refactor client with jquery + css

838c8de

jerjou force-pushed the firetactoe branch from c7a843f to 838c8de Compare October 7, 2016 21:17

theacodes approved these changes Oct 7, 2016

View reviewed changes

jerjou merged commit 988acf5 into master Oct 7, 2016

jerjou deleted the firetactoe branch October 7, 2016 21:40



		def _send_firebase_message(u_id, message=None):
		global _FIRE_URL

		payload, RSA.importKey(credentials['private_key']), 'RS256', exp)


		class Wins():

		o_wins = map(lambda s: re.compile(s), o_win_patterns)


		class Game(db.Model):



		def _send_firebase_message(u_id, message=None, _memo={}):
		if 'http' not in _memo:

Add firebase-based tic-tac-toe sample. #560

Add firebase-based tic-tac-toe sample. #560

Uh oh!

Conversation

jerjou commented Oct 6, 2016

Uh oh!

jerjou commented Oct 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerjou Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theacodes Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerjou commented Oct 7, 2016

Uh oh!

jerjou Oct 6, 2016 •

edited

Loading

theacodes Oct 7, 2016 •

edited

Loading