Vanilla #31

callum-oakley · 2018-02-13T14:28:46Z

The majority of the functionality of the typescript library has now been translated to vanilla js and refactored along the way. Probably worth getting some feedback on this as it is before committing to the finishing touches.

API changes:

Promises instead of callbacks wherever that makes sense.
The existing "delegate" functionality is basically the same, but I'm proposing we call them "hooks" instead, since it seems like more familiar terminology in the javascript ecosystem.
subscribeToRoom will also join room if you're not already a member
consistent type checking of arguments
chatManager.connect doesn't resolve with the current user object until the full initial state is known (the initial user request has already completed, presence subscription has been established etc)
a smattering of method renamings and "was a room but now a roomId"

See luke's demo modified to use this branch: lukejacksonn/react-slack-clone#1

Internal changes:

RIP typescript
rollup instead of webpack, for no real reason other than that it's what I'm familiar with, and the build process was changing anyway with the move away from typescirpt. If this turns out to be a problem (for react native etc) I'm happy to move back to webpack.
abstract a lot of the async logic of "get this thing from the api unless we have it already, in which case just return the one we've already got" in to the user / room / presence store where before it was done case by case
(hopefully) general refactoring and simplifying

Tests

The integration tests that currently exist were super useful for doing the rewrite, and will continue to be useful to know we haven't broken stuff. But they aren't very good at testing failure paths, and won't scale much more (they already take >60s to run!). So we might want to unit test things too? I was loathed to do this immediately, but it might make sense once we've more or less settled on a good internal structure.

Remaining work

cursors support
rich media support
everything that currently has a TODO in the code
improved logging
~~make sure the error handling is watertight (it certainly isn't at the moment)~~ still not, but probably "good enough for now"
a corresponding PR for the docs to reflect the changes to the public API
react native and ~~worker builds~~
allow opt out of presence / cursors, etc
detailed changelog

Build

yarn lint:build

Run the tests

yarn lint:build:test

hamchapman

GG this looks great. I can't even remember all of the little comments I left as I went but maybe some of them will be helpful!

hamchapman · 2018-02-13T14:57:16Z

src/current-user.js

+  /* public */
+
+  get rooms () {
+    return values(this.roomStore.snapshot())


[room|user]store.snapshot is schweeeeet ❤️

hamchapman · 2018-02-13T14:59:26Z

src/chat-manager.js

+    })
+    return Promise.all([
+      currentUser.establishUserSubscription(hooks),
+      currentUser.establishPresenceSubscription(hooks)


Presumably with the way it's set up at the moment, if the presence subscription fails then the promise returned from the call to connect will end up in the catch?

I think this is how I have it in the Swift one at the moment as well, but I'm not completely sure about it. I think it makes the most sense, but more so when we have the ability to disable presence and cursors, if a user so wishes

Yeah good point, if either fails we'll end up in the catch. This is less of an issue when you can disable presence, but even then... if someone has everything on by default, would it be better to go on even if the presence subscription fails on the off chance the client isn't using any of the presence stuff? If they are it would break stuff later on, but maybe partial functionality is better than no functionality?

It wouldn't be a big change, we could just have establishPresenceSubscription always resolve, and the presence stuff would be undefined and users could handle that however they want... 🤔

I'm loath to add more config but maybe we could have something akin to a strict mode, which is false by default, but it can be set to true if someone wants the behaviour of "if any of the initial connection requests fail then I want the catch block to be hit of the connection promise".

Then we can have the default being that as long as the user subscription succeeds, we resolve the promise. WDYT?

might be a good compromise yeah. I'll make a card to do the opting out of presence / cursors etc stuff and include this as part of it. We don't necessarily even have to document it but it would be nice to have if customers were like "wth everything broke horribly because the presence sub failed". That being said... if we're not going to document it, maybe we should just add it later if someone actually runs in to that problem. hmmmm

hamchapman · 2018-02-13T15:08:51Z

src/current-user.js

+      }
+    })
+      .then(res => {
+        const basicRoom = parseBasicRoom(JSON.parse(res))


Why a basic room and not a full room? Maybe this gets handled by adding a basic room to the room store - I've not gotten to that yet though!

yeah, the this.roomStore.set(basicRoom.id, basicRoom) below returns a full Room :)

hamchapman · 2018-02-13T15:09:37Z

src/current-user.js

+    return this.apiInstance
+      .request({
+        method: 'GET',
+        path: `/users/${encodeURIComponent(this.id)}/rooms?joinable=true`


encodeURIComponent could probably just be called once at the point of initialisation to save doing it every time there's the id appearing in a path

good shout 👍

hamchapman · 2018-02-13T15:11:01Z

src/current-user.js

+      })
+      .then(() => this.roomStore.pop(roomId))
+      .catch(err => {
+        this.logger.warn(`error joining room ${roomId}:`, err)


joining -> leaving

hamchapman · 2018-02-13T15:14:06Z

src/current-user.js

+        )
+        return this.userStore.fetchMissingUsers(
+          uniq(map(prop('senderId'), messages))
+        ).then(() => sort((x, y) => x.id - y.id, messages))


Need to figure out what's going on here - it looks clever, but haven't worked it out yet 😛

hamchapman · 2018-02-13T15:14:48Z

src/current-user.js

+    typeCheckObj('hooks', 'function', hooks)
+    messageLimit && typeCheck('messageLimit', 'number', messageLimit)
+    // TODO what is the desired behaviour if there is already a subscription to
+    // this room? Close the old one? Throw an error? Merge the hooks?


Excellent question - I think the easiest thing for us would be to close the existing subscription and just replace the whole thing

That seems like a good solution, in that case this depends on proper subscription cancelling and then it should be easy. I'll put it on the list!

hamchapman · 2018-02-13T15:18:11Z

src/message.js

+    this.text = basicMessage.text
+    this.createdAt = basicMessage.createdAt
+    this.updatedAt = basicMessage.updatedAt
+    this.userStore = userStore


Not sure if the Message having a room and user store is genius or madness. Probably simplifies the "enrichment" process a lot, but then I can imagine there being a lot of getSync calls happening which makes me squirm a bit. Although I suppose that has to happen whether you get the sender or room later or at the point of message creation.

Yeah the reasoning was that I wanted to keep all the mutable state in the three "stores", and then everything else is always kept up to date for free. I don't think it should be too much of a performance hit, since getSync is O(1), but something to try if things get slow though for sure :)

hamchapman · 2018-02-13T16:24:56Z

src/store.js

+  pendingGets = [] // [{ key, resolve }]
+
+  initialize = initialStore => {
+    this.store = clone(initialStore)


Why clone and not store the initialStore?

Hmmmm. The resoning when I wrote this was probably something like: if the consumer mutates initialStore after initialization then that could fuck everything up, buuuut since we only do all the initialization intenally we probably needn't bother.

hamchapman · 2018-02-13T16:56:22Z

src/utils.js

+// appendQueryParam :: String -> String -> String -> String
+export const appendQueryParam = (key, value, url) => {
+  const [ before, after ] = split('?', url)
+  return before + '?' + (after ? after + '&' : '') + urlEncode({ [key]: value })


callum-oakley · 2018-02-20T17:46:28Z

src/current-user.js

  }

  /* internal */

+  uploadDataAttachment = (roomId, { file, name }) => {
+    // TODO some validation on allowed file names?
+    // TODO polyfill FormData?


We'll have to do this at some point (or use lower level methods), but the existing JS sdk uses FormData as well, and the current goal is to match functionality with what already exists asap so that we can get ths released.

iMerica · 2018-02-20T22:59:53Z

"delegate" functionality is basically the same, but I'm proposing we call them "hooks"

+1 for renaming delegate to hooks. 'Delegate' is popular in the Obj-C/Swift scene, but not as intuitive in the JS world.

mdpye · 2018-02-22T11:01:18Z

tests/main.js

+    .then(() => t.end())
+    .catch(endWithErr(t))
+  t.timeoutAfter(TEST_TIMEOUT)
+})


Are we missing a case for data attachments?

@mdpye yes! my bad, going to add one now. (will cover fetchAttachment too)

callum-oakley · 2018-03-01T11:49:35Z

Do we need to ship with updateRoom and deleteRoom? I really think this is a server concern (and you can't do either with default permissions). wdyt @hamchapman ?

hamchapman · 2018-03-01T11:54:01Z

I'm tempted to keep them because I can see it being something that an admin-type user might want to be able to do from the client.

Is there a reason to remove them other than them requiring permissions elevated from the default?

callum-oakley · 2018-03-01T11:58:16Z

nah not at all. Just trying to narrow down the list of "final bits and bobs" to do before releasing this. It's a short job to add them :)

hamchapman · 2018-03-05T11:49:34Z

updateRoom & deleteRoom changes LGTM 👍

…ed tests

harrisrobin · 2018-04-19T11:53:03Z

@callum-oakley pretty random but purely out of curiosity, what was the reason to move away from typescript internally? :)

Cheers 🍺

callum-oakley · 2018-04-19T13:17:39Z

replied in slack :)

callum-oakley requested a review from hamchapman February 13, 2018 14:28

callum-oakley mentioned this pull request Feb 13, 2018

Async await #20

Closed

hamchapman approved these changes Feb 13, 2018

View reviewed changes

callum-oakley commented Feb 20, 2018

View reviewed changes

callum-oakley force-pushed the vanilla branch from 4652283 to 3564f6f Compare February 21, 2018 14:10

mdpye reviewed Feb 22, 2018

View reviewed changes

callum-oakley changed the title ~~[WIP] Vanilla~~ Vanilla Mar 1, 2018

hamchapman mentioned this pull request Mar 7, 2018

Subscribing to already subscribed channel #43

Closed

callum-oakley force-pushed the vanilla branch 2 times, most recently from a1eb87a to 0992758 Compare March 13, 2018 15:10

callum-oakley added 15 commits March 13, 2018 15:29

blank slate

fcef9eb

tests passing up to and including added to room hook

b346f51

removed .prettierrc

7dd6aa2

basic user and presence store

f1d5976

presence updates

6b37c3c

typing indicators, plus function style consistency

f2b926c

room store

d3ea427

make Store.set async

b1b95a4

removed from room, pop store

992b27e

get users (wip)

b41a60f

pull user subscription out in to own class

b1790de

split presence subscription out in to own class

c9b1447

simplify presence sub onEvent

3f56054

clean up userSubscription onEvent

2c05ffe

user left hook

3726718

callum-oakley and others added 25 commits March 13, 2018 15:29

0.7.0-alpha.2

4a41fd1

enable fetch data attachment test

7998301

Use the presence service for presence-related things

e014749

make switching between environments while testing easier

0da726e

cancel method on subscriptions

f015497

0.7.0-alpha.3

881e223

test userWentOffline hook

5bcc0c3

minimal changes to get existing example working in firefox

8f08e3b

Bump pusher-platform dependency to 0.15.0 and unskip previously skipp…

0194589

…ed tests

Fix CSS in example

2d4a602

don't lint example

89b1d93

add some missing timeouts

8b86e27

0.7.0-alpha.4

04c569c

only make a single token provider request at a time

d7db4dc

0.7.0-alpha.5

775f144

queryPrams and headers options for TokenProvider

42e92d0

0.7.0-alpha.6

bc2f2ab

react native build

8d1d3d1

inline test babel config

2bad618

addUser -> addUserToRoom; removeUser -> removeUserFromRoom

2787635

functions all recive a single object parameter

c9a696f

append 'on' to all hooks

83f1545

0.7.0-alpha.7

f282852

changelog, 0.7.0

36b73c5

point to @pusher/chatkit in readme

b77674a

callum-oakley force-pushed the vanilla branch from d34a86d to b77674a Compare March 13, 2018 15:30

callum-oakley merged commit 31e18b5 into master Mar 13, 2018

callum-oakley deleted the vanilla branch March 13, 2018 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vanilla #31

Vanilla #31

callum-oakley commented Feb 13, 2018 •

edited

Loading

hamchapman left a comment

hamchapman Feb 13, 2018

hamchapman Feb 13, 2018

callum-oakley Feb 14, 2018

hamchapman Feb 14, 2018

callum-oakley Feb 14, 2018

hamchapman Feb 13, 2018

callum-oakley Feb 14, 2018

hamchapman Feb 13, 2018

callum-oakley Feb 14, 2018

hamchapman Feb 13, 2018

hamchapman Feb 13, 2018

hamchapman Feb 13, 2018

callum-oakley Feb 14, 2018

hamchapman Feb 13, 2018

callum-oakley Feb 14, 2018

hamchapman Feb 13, 2018

callum-oakley Feb 14, 2018

hamchapman Feb 13, 2018

callum-oakley Feb 20, 2018

iMerica commented Feb 20, 2018

mdpye Feb 22, 2018

callum-oakley Feb 22, 2018

callum-oakley Feb 22, 2018

callum-oakley commented Mar 1, 2018

hamchapman commented Mar 1, 2018

callum-oakley commented Mar 1, 2018

hamchapman commented Mar 5, 2018

harrisrobin commented Apr 19, 2018

callum-oakley commented Apr 19, 2018

Vanilla #31

Vanilla #31

Conversation

callum-oakley commented Feb 13, 2018 • edited Loading

API changes:

Internal changes:

Tests

Remaining work

Build

Run the tests

hamchapman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iMerica commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

callum-oakley commented Mar 1, 2018

hamchapman commented Mar 1, 2018

callum-oakley commented Mar 1, 2018

hamchapman commented Mar 5, 2018

harrisrobin commented Apr 19, 2018

callum-oakley commented Apr 19, 2018

callum-oakley commented Feb 13, 2018 •

edited

Loading