spencercarli / react-native-meteor-boilerplate

MIT License
628 stars 139 forks source link

ddpClient is undefined after hot module replacement #15

Closed faceyspacey closed 8 years ago

faceyspacey commented 8 years ago

I'm wondering if you've come across this and what you recommend to fix it? I'm pretty sure it has to do with the fact ddpClient is instantiated via new DDPClient.

I'm thinking of doing something similar I did with a redux store:

  module.hot.accept(() => {
      const nextRootReducer = combineReducers(require('./reducers'));
      store.replaceReducer(nextRootReducer);
      store = store; //for some reason the original store becomes null after the reducer is replaced.
    });

Basically you need to clean up the mess in module.hot.accept.

spencercarli commented 8 years ago

I've never tried this. Could you give some steps (or links) to how to use it? I know hot reload exists in RN now but I've never tried it

faceyspacey commented 8 years ago

They actually have an in-depth walkthrough which I stumbled upon on the blog (probably should become part of their main docs at some point):

https://facebook.github.io/react-native/blog/2016/03/24/introducing-hot-reloading.html

...Anyway, what they prescribe in that article allows you to hot reload reducers, which is great, but if you happen to be engaging in the "anti-pattern" of using store.getState() or store.dispatch() directly, store will become undefined. So above is my fix for it. Just reset the value you have for store, which I have in a closure and return from an exported function I can use in other files: getStore() { return store }.

So I'm assuming we need to do something similar with ddpClient. I haven't done it yet because my solution here is somewhat hacky, and I'm thinking their could be a better more standards-based approach. I'm really just wondering why in both the redux case and in the ddpClient case they become undefined. What about the module system doesn't just re-run the code? And in addition, what if there is code we don't want re-run? How exactly does module caching work when multiple files import them? I assume all the code isn't re-run, but the exports are cached, and somehow cyclical dependencies are resolved. So then, if they're cached when different files import the same file--I assume with hot module replacement you simply have no choice but to re-run the code again, so why is DDP client becoming undefined? Anyway, there are answers out there, and I've read them in the past, but I just don't have much incentive at this very moment to figure out the idiomatic approach to address this.

Basically, my guess as to what's happening is this:

if I have:

const store = configureStore();

export function getStore() {
   return store;
}

And then you'd think if the file got re-run as part of hot module replacement, getStore would be replaced throughout the rest of the app. WRONG. It's not, and it's trying to return an old store which now doesn't exist. The module system + hot module replacement system simply doesn't update all references to exported functions. Some exported functions will use the cached version, whose closure somehow killed the variable in reference.

faceyspacey commented 8 years ago

I also am pretty sure that exporting a function that returns ddpClient would solve the problem. I just don't wanna go around replacing my code that uses it yet. Basically it would look like this:


function setupDDPClient() {
    const ddpClient = new DDPClient(config.ddpConfig);

    ddpClient.sha256 = (password) => {
       return {
          digest: hash.sha256().update(password).digest('hex'),
          algorithm: "sha-256"
       };
   };

   //etc

   return ddpClient;
}

let ddp = setupDDPClient();

export default function getDDPClient() {
      return ddp || setupDDPClient();
}

so in production for users it would likely never be re-created. But in developer if some leaky underlying abstraction--such as hot module replacement--breaks it, you're guaranteed to get it again. That should 100% solve it.

The caveat being in other files, you have to change your code to actually call the exported function rather than just start using it, which is probably a more idiomatic approach than something relying on the new operator which you basically aren't seeing in the idiomatic wave of react and module-pattern-based code that has taken over. And I'm not saying don't use new DDPClient. I don't feel like messing with what's under the hood over there, but just that my hunch is that also is contributing to the problem. So basically we keep it, wrap it in a more idiomatic module (that catches the case where it becomes undefined).

faceyspacey commented 8 years ago

or we could just make it a standard class that React seems prepared to hot module replace properly:

export default class Ddp extends (new DDPClient(config)) {
     sha256() {

     }
     callPromise() {

     }
}

I gotta see exactly how to make that work, as far as the first extends line goes. But in React Native project, I rather be looking at just another class. New developers to the project tend to freak out when they see a file that looks like the current DDP file when all the other files look exactly the same, i.e. react classes. So if giving ddpClient that veneer also solves the hot module replacement, that's definitely the way to go from my perspective.

faceyspacey commented 8 years ago

...well it turns out--as you obviously know since you built it--ddpClient is itself an ES6 class:

https://github.com/spencercarli/node-ddp-client/blob/master/index.js

so we just extend it once again, and perhaps do what I did above with setupDDPClient but for a single line that instantiates it. Or instantiate it in every file that uses it, which is less than ideal, but ultimately fine for me since I only use it in my redux actions, which is what I deemed the "idiomatic" approach to go with, i.e. instead of calling the API directly outside of async redux actions. So I don't have many component files littered with ddpClient. They all just use redux actions. ...But perhaps the problem is that it's a singleton maintaining shared state for such things as collections, etc. ..Yea, so you basically do have to use it as a singleton, and hot module replacement may somehow cause it to lose all that internally stored state. Cuz you can re-instantiate it, and have it available, but it won't have your collections, etc. I happen to not be using any of that stuff (yet), and just making basic api requests, and the login token you have setup with AsyncStorage, so basically it's internal properties such as _isConnecting which may be problematic for my simple use case.

...module.hot.accept likely needs to be used to transfer the old state to the new instance (or instead somehow preserve the old instance).

I'm also thinking we should make it its own github repo. I.e. this file:

https://github.com/spencercarli/react-native-meteor-boilerplate/blob/master/RNApp/app/ddp.js

I've added a lot more promises to it. For me, it was the most useful thing from the boilerplate. I mean, forgetting the minimongo stuff, it _is_ Meteor + React Native. And given what my guess is that most people will wanna use Redux with React Native rather than minimongo and something like https://github.com/ultimatejs/tracker-react since it's the more idiomatic approach, well, then this file certainly is all you need to get a Meteor backend running in your app. Although Redux is more work, it makes up for it in the predictability and insight its dev tools provide. I'm still on the fence on Tracker vs. Redux in React Native--or perhaps I'm not since, I chose plain Redux. But really it's not even a decision, since you can use both in the same app. I only have one realtime part of my app--a support chat. My initial plan has been to build it with the above file and subscribe and manually update redux, but I could just as well use minimongo and something like TrackerReact (if it works in React Native). How is minimongo support by the way? I saw you had worked on it in the react-native-meteor project the other day. Can we use minimongo/tracker in React Native yet?

faceyspacey commented 8 years ago

...another thing to note is this hot module replacement issue only pertains to working on the ddp.js file itself. Basically if we complete the API and make it its own package, it doesn't need to be used. So this whole problem becomes a non-issue (for everyone but us lol, if we wanna make more changes to it).

Either way, this is some solid stuff for using Meteor with React Native. We should complete the API in a separate package and add some examples for usage with Redux, if not straightup build in support. It can keep a callback API, but the promise-based one should be promoted since that's what works nicely with redux-thunk and is the idiomatic Redux way.

Nothing else is needed than this to use Meteor as a backend with React Native. And honestly most people are gonna pass on the tracker + minimongo stuff at this point. So it has the potential to become the goto Meteor package for React Native. But positioned as it currently is positioned as part of a "boilerplate" isn't doing it justice. A suped up github repo of its own, submission to js.coach, etc, etc, is all it needs become popular and give Meteor people confidence in React Native. Honestly, React Native has been beautiful and using this package along with some more promises has been a beautiful way to use Meteor in React Native. So any concern about the combination of the two needs to be squelched, and people need to get on this. That phonegap garbage is dead. So we can do a lot to build confidence in the react native path forward for Meteor folk. You already have. Let's take it up a notch.

ps. As far as react-native-meteor by the in progress team, that's great and all, but the React community is bigger and more defining of the trends than the small Meteor community--as the Meteor community has recently and abruptly learned--and they aren't gonna wanna use that. Sorry in progress team, I've been in that boat too. But the React world will wanna use Meteor as a backend, especially with the recent 1.3 module enhancements etc. Things like this, marketed towards React-first developers (rather than Meteor-first developers) I predict will do well. And, it will do well with Meteor-first developers as well because most are quickly learning that it's just best to jump on the Redux bandwagon. Or if it's not "best," it's at least something they wanna try so they at least know what the idiomatic approach is. And that's the point, this is positioned as the idiomatic approach of combining Meteor + React Native, whereas react-native-meteor definitely isn't idiomatic as long as it's using Tracker. ...Just to give you some background, I initially created Tracker React and am the guy who wrote this: https://medium.com/@faceyspacey/tracker-vs-redux-implicit-vs-explicit-df847abcc230#.afnnnec58 among other articles back when people were all concerned about React making Blaze obsolete in the fall. So I'm someone that wanted to see Tracker live. After working deeply for several months with React and Redux--it's just superior, end of story. The functional composability of components rules all. Redux definitely is a lot more setup, but its declarative stability and observability are paramount. So I conclude that ddpClient is the idiomatic way to combine Meteor + React Native, not anything that uses Tracker. ...there could be ways to use Minimongo or at least its selectors with Redux (and data stored into it via ddpClient.subscribe/observe, but we'll save that for another day...

spencercarli commented 8 years ago

Thank you for such a detailed response! I really appreciate you taking the time to write this - I'm still digesting it all apologies if my response seems short 😄

A few things I took from this:

Initially I had budgeted some time to transitioning this boilerplate to react-native-meteor but I think you made some great points so I'm planning to put that time towards diving into the various things outlined here instead.

I also want to dive into integration with redux - I use it in the client apps I build but as you said, there is a lot of boilerplate.

As for your question pertaining to minimongo on RN - I haven't used it in quite a while. The last few apps I've built have all been redux based and I just use normal array operators to search for the relevant data. It seems that react-native-meteor has pretty solid support for it though. I'm only using it for the accounts system though at this point.

Okay I'm going to jump into the code a bit and see what I can figure out. Again I appreciate all you put into your comments!

faceyspacey commented 8 years ago

you should create the repo since you're the real creator--here's a gist of what I have:

https://gist.github.com/faceyspacey/5c00a477c1e70d4a3bfe3d045994dcb3

so there's some additional promises for you, a little bit of refactoring. this._onAuthResponse is made to throw the error from the async requests so all the .then/catch() lines can be one line.

loginWithTokenPromise also has an additional feature where you can pass the token manually if you want.

Basically, the direction it's heading is we need to make wrapper methods for meteor's entire Accounts API that trigger requests to the server.

One issue that would be nice to handle for people that just jumped through hoops to handle is this one:

https://forums.meteor.com/t/how-to-manually-generate-reset-password-token-for-use-in-react-native-app/22315

...it's the forgot password scenario. I wanted to know how to generate the reset password token manually without having to send an email. I didn't get that answer there, but just solved it myself in a way that is hopefully good enough. Basically i don't want user's having to go to their email app and click a link because that's bad user experience having the user leave your app (possibly never to return). The way I circumvented that was by emailing a short code which they can see in the subject line shown in a push notification, and then enter without ever leaving the app. ...The other complex issue is that the link would lead to a website, which (many crappy apps still having you juggling between a reset password page on a mobile webpage and your app), but if done correctly would then require you to setup Universal Links (e.g. a for iOS a file on your server that Apple can parse and determine to send the user directly to your app rather than a webpage), and then parse the reset password token from the URL redirected to your app. So anyway, what I do is confirm the 5 digit pin code they were emailed on my server when they submit it, and then generate a temporary password, and send it to them, and then automatically log them in and redirect em to the password reset page where i call the standard Accounts.changePassword(oldPassword, newPassword) method.

Without going all the way to offer a solution for that, we can't call this complete. We could package up the code I have on my server so they can just add it as a package to their meteor app. I'm sure security experts would frown on it though, being that I send a password (although a long generated one) over the wire from the server back to the app to be used. There's no expiration on it either, which combined with the fact that it's not actually sent through email, may be the only other difference between what I do and what the standard Meteor accounts reset password token sent through email does. We could hack the expiration by storing a date on the user model when it was created and then in Accounts.onLogin check that they aren't within the window they are allowed to use it.

Another custom mobile-oriented thing I have is verifying their email on signup. Again, rather than have them go to their email, I email them a 5 digit code, and also send it to the client, and then compare it on the client without even having to go back and compare it on the server. My thinking here is that more security isn't needed because they wouldn't be able to use it to fake a forgot password scenario and get into somebody else's account. You ultimately won't be able to create your account if one already exists. So it's just a nice user experience while not letting user's enter the wrong email address.

Anyway, food for though. These are things we could get really right for mobile and help people avoid the often typical wasteland of authentication work you have to do.

...If we could come up with a few redux helpers to automatically store their subscriptions in reducers, we'd have a real offering here. What I'm thinking is this: we use the replaceReducer part of the code i pasted above:

module.hot.accept(() => {
      const nextRootReducer = combineReducers(require('./reducers'));
      store.replaceReducer(nextRootReducer);
      store = store; //for some reason the original store becomes null after the reducer is replaced.
    });

and dynamically add/remove reducers at runtime based on which subscriptions you have! Then you can just subscribe in componentWillMount by dispatching an action, which is wired via async style promise code to make the DDP subscriptions/observations, and return the results into a reducer by the same name as the subscription, which we generate on the spot. You can also provide an additional parameter to ddpClient.subscribe() to specify a name for the reducer--i.e. for cases that you are using the same subscription in multiple places throughout the app. At this very moment, I forget if you can even have multiple subscriptions to the same subscription, i.e. with different parameters--but if you can, the aforementioned solution would address that.

Here's how it would look:

import * as actions from './actions';

class MyComponent extends React.Component {
   componentWillMount() {
       this.props.subscribe('posts', [categoryId, limit, page]); //u obviously get these params from somewhere
   }
   render() {
      <View>
         {this.props.posts.map((post) => <View><Text>{post.title}</Text</View>)}
      </View>
   }
}

export default connect(
   ({collection}) => ({posts: collection.posts}),
   actions
)(MyComponent)

/** reducer **/

export function collections(state={}, action={}) {
  switch (action.type) { 
    case types.SUBSCRIPTION_FETCING:
      return {
         ...state,
         [action.collection]: false,
      };
    case types.SUBSCRIPTION_ADDED:
      return {
         ...state,
         [action.collection]: action.payload,
      };
    //case types.SUBSCRIPTION_CHANGED/ETC...
    default:
      return state;
  }
}

/** action creator **/

export function subscribe(subscriptionName, params) {
   return (dispatch, getState) => {
      dispatch({type: +'SUBSCRIPTION_FETCHING', collection: subscriptionName});

      return ddpClient.subscribePromise(subscriptionName, [params])
        .then(() => {
            ddpClient.observe(subscriptionName);
            observer.added = function(id) {
               dispatch({
                  type: 'SUBSCRIPTION_ADDED', 
                  collection: subscriptionName, 
                  payload: ddpClient.collections[subscriptionName], //um, prolly wanna do something more immutable
               });
            };
            //observer.etc
        })

  };
}

and obviously while creating it I changed my mind against a dynamic reducer (at least for this quick example--we could dynamically create them and provide a better user experience where you don't have to go through post.collections.posts and just use props.posts, which may also provide better performance). Anyway, it's just one reducer that stores all the collections similar to node-ddp-client. One thing to note is there is no notion of a subscription vs an observer. They are the same and that would definitely provide a smaller API surface area, and therefore better developer experience for most use cases. The only reason to have both concepts is so different views/components can use different slices of the same data, which seems to be less common in small-screened mobile apps. It also proved to be a very dangerous concept in Meteor where could end up with different templates querying all data in a collection rather than just in a subscription, since there never was a concept of query models from a specific subscription. Basically you could end up with overlapping subscriptions providing data from the same collection, and then in your templates you want to query rows from just one of the subscriptions, but you get data from both subscriptions. The obvious positive in the tradeoff here is that you get less requests. You make one subscription and the components basically already have the data. Of course you can mimic that by passing the data down as props, etc, etc. There are various tradeoffs here, which ultimately Relay specializes in--though I'm waiting for the day that picks up as much steam as Redux has. ..So that all said, we could get a lot of mileage by just doing what I showcased above where you subscribe/observe in one go and there's no concept of slicing/observing part of the data form a subscription with a sub-selector. That's basically the perfect V1 for this tool. And on top of that we don't need to risk any issues with dynamically generating reducers. Basically we just require that you import our staple reducer(s) into your configureStore function where you call combineReducers and for the subscribe action, you just import it into the components that want to use it and bind it, etc. Or import it into your actions file, and re-export it so it goes along with all your other actions if you do like I do and just give all your components all your actions (which I'm still not sure whether there is any performance indications for doing, but my hunch is since they are just constant functions, No).

I think we got a mini holy grail here that will make React Native highly attractive to Meteor developers. And it's really easy to complete this. Now it's just about finding the 20 hours it will take.

faceyspacey commented 8 years ago

...also, it's worth noting this looks almost identical to what you would do with getMeteorData or TrackerReact which use Minimongo, except it's better in that you get access to componentWillReceiveProps and other such callbacks rather than forceUpdate arbitrarily being used. So that means you can properly animate things and whatnot.

But perhaps more importantly, the result in your component code is literally the same developer experience: receive collection models, iterating through, displaying data off of them based on a subscribe call.

It's all truly functional. Or at least comparable to anything else going on in Redux. ...So what do you lose when dropping the non-functional Tracker mini-mongo stuff?? Well, you lose the ability to use mongo selectors in the client. I mean you could pass them as params to meteor publish methods, but you get the idea--you're restricted to how you query the data server side in the publication. That means--like Redux--you have some upfront boilerplate before you can start accessing your data, i.e. you'll likely have more publication methods, rather than just a few serviced by multiple client-side queries/observers/cursors. But just like with Redux itself, you make up for it via things like the devtools where you can see every single action, i.e. every model added to a subscription, roll em back, etc.

At the end of the day, for very many apps and uses cases, it's very close. A lot of apps can do just fine with all the querying restricted to the publication. I mean it's perhaps a better more explicit way to go in general. You avoid pitfalls of overlapping subscription data. And if you want to use the subscription across multiple components, you can provide a broader set of data, and then manually slice it in the sub-components.

That all said, a V2 could allow you to dispatch selectors which use code that might be easy to extract from minimongo (or elsewhere) where the selectors are used to filter models out of an array. There's probably a chunk of code somewhere that does nothing but exactly that. And boom we just add it to our reducers. If this package becomes popular that will be inevitable.

...But anyway, my point was just that for all the people still in Blaze land or forcing the use of minimongo into React, we have an interface here that is both very closed to what they are used to and idiomatic React/Redux, bringing you all the benefits of the latter. It's the perfect combination of the two, and perhaps most importantly: it's in React Native!!!

faceyspacey commented 8 years ago

...yea dynamically generating the reducers is probably the way to go, so we arent iterating through a collection of collections and immutably producing another such collection of collections every time, which will get heavy performance-wise the more collections and models you have (though maybe the models don't have to be truly pure). That's ultimately the secret sauce that sets this apart from what developers with minimal knowledge of replaceReducer can do themselves. Dynamically generated reducers baby! So we just gotta test whether we can in fact do that. I'm pretty sure we can since HMR does exactly that.

we call this as a side effect in our subscribe action:

function onSubscribe(store, userReducers, generatedReducers) {
      const nextRootReducer = combineReducers({...userReducers, ...generatedReducers});
      store.replaceReducer(nextRootReducer);
}

Then we just figure out a nice interface for users to pass us their store and userReducers objects.

spencercarli commented 8 years ago

Hey @faceyspacey I really appreciate your detailed response and enthusiasm! Unfortunately I'm struggling to fully grasp what you're trying to convey here... but none-the-less I think it's probably a task larger than I'm personally prepared to invest in right now as I've started to transition to react-native-meteor

I think you made some interesting points on the pitfalls of that route and the lack of adoption by a greater community because of it's dependencies...

I'm curious is the solution you're suggesting basically like a more simple react-native-meteor? For example it provides a nice interface to tap into the capabilities Meteor has but without the Tracker dependency and leaning more on Redux?

It would be interesting if react-native-meteor could be abstracted into a package that provides the various methods and one that provides mixins like getMeteorData and likewise one that handles some Redux related stuff.

I'm currently working on a project that is using the methods provided by react-native-meteor but leverages Redux for everything. I'm hoping that it will be a good learning experience that I can bring to the community.

If you want to try implementing any of the things you mentioned please let me know and I'd be happy to do some reviewing.

faceyspacey commented 8 years ago

yea, leaning more on Redux. Basically drop all the Meteor stuff, except for what's in your package, e.g. subscribePromise. Generally replicate Meteor collections in Redux stores, losing the ability to use ad hoc mongo queries. That's what I see the direction of Meteor going for Meteor-second, React-first developers. Though Apollo could change things, but it just seems like--even tho its truly open source unlike past MDG endeavors--it's too much of a departure from Relay and Redux, which the greater NPM community will probably stick to. I was hoping they'd just enhance Relay for the client-side aspects, but it's seems to have become it's own competing monolithic framework that basically comes with Meteor lock-in. In terms of "lock-in" it's better than the past, but my hunch is the greater NPM community doesn't take to it, and that's a major mistake on their part. So that said, the Redux + Relay path forward will continue to be "the one true way." Redux is supposed to get replaced with a superior Relay that does what Redux does (i.e. a focus on "UI state" instead of just "domain state"). So I imagine the Facebook guys will bring something to the table that brings it all together at some point. Until then, everyone's using Redux. So that's where you wanna be.

In short, neither of us have time to build this--as it easy it is--but now's it's time to shine in the market, as by the Fall you'll want to use something else. But that's a serious amount of time in the development world. Meteor + React Native projects should be using this today. That said, cuz of the pull to refresh user experience that is natural and commonly understood for Native apps, realtime means a lot less in native apps. Most mobile apps only need realtime for a small portion of their app that needs chat or something. That's why Meteor dropped the whole focus on realtime everything. Nobody needs that. And when you do need that, you wanna do it in a scalable fashion (because you're whole app is about that realtime feature), which Meteor was never providing. They completely missed the "sweetspot." So that's the catch-22 and realization they had that motivated Apollo, which does support "realtime" for those that want to put the effort into optimizing it, but not in the same "realtime everything" capacity.

So that leaves just ad hoc mongo queries--or there lack of--as the only con of this equation. And given all you get in predictability and observability of your system, implicit queries strewn throughout front-end code probably is best to see out of the picture. You don't need that, you don't need realtime, therefore you don't need Tracker, getMeteorData, automatic synced client-side collections or any of that stuff. But what is needed is first class support for Meteor backends consumed by Redux clients.

smooJitter commented 8 years ago

Amen!!!!

spencercarli commented 8 years ago

Made the switch to react-native-meteor so I'm going to close this. I appreciate all of the valuable discussion here!