faceyspacey / require-universal-module

Universal-Rendering Module Loading Primitives
MIT License
28 stars 3 forks source link

it('should respect network conditions') #8

Open theKashey opened 7 years ago

theKashey commented 7 years ago

Just playing with universal-modules and was surprised by default behavior: if loading was failed, due to any reason - timeout or network failure - it will never let you repeat operation. And, if you fix that with alwaysUpdate flag - it will always load new data from a server. Nothing in between.

What do you think about adding something to handle situations like this?

faceyspacey commented 7 years ago

I agree. I think a forceUpdate mechanism is a good idea.

requireAsync takes an array of arguments. The last one could be an options with { forceUpdate: true } setting the capability you want.

How To Implement It: The last element of the args array should be options here: https://github.com/faceyspacey/require-universal-module/blob/master/src/index.js#L116

and it should be removed from the array before the args is passed to the userland asyncImportfunction.

You should give it a try as a PR. It's not a big module.

faceyspacey commented 7 years ago

here by the way is where the userland function eventually gets called:

https://github.com/faceyspacey/require-universal-module/blob/master/src/index.js#L150

if it's not clear what we're doing here--basically we are supporting several different usages:

so in the latter 2 cases, they look similar, which is a bit weird since one deals with promises and one does not. They both however must somehow call resolveImport. In the case of the one that returns a promise, we do it for them, and in the case of the one that doesnt return a promise, the user must do it.

So if a promise is returned, we do it:

https://github.com/faceyspacey/require-universal-module/blob/master/src/index.js#L155

...once u understand how the resolveImport function works here, it should make sense what requireAsync is doing, and how u can just receive an arg and NOT continue to pass it on to resolveImport. The requireAsync function is otherwise very short, so it should be easy to figure out what to do with it. That's the idea.

Use forceUpdate here: https://github.com/faceyspacey/require-universal-module/blob/master/src/index.js#L121

but if it already exists here: https://github.com/faceyspacey/require-universal-module/blob/master/src/index.js#L117 just return mod.

Thanks for your other PR by the way in RUC.

theKashey commented 7 years ago

Ok, I`ll return with PR next week. Look like it is a bit easer in case or removing arguments - why I should add forceUpdate to requireAsync then I can read it from options like alwaysUpdate?

PS: As long options from RUC just passed down into RUM - you one mention RUM flags in RUC?

faceyspacey commented 7 years ago

...yea RUC is a little behind. it's just initialRequire and alwaysUpdate. i think lets leave it out for now. i added them for using RUM directly (something else i was doing). ...but maybe in a later version of RUC (i dont want to bloat the API just yet).

...ps i think ur right--just a regular option for forceUpdate and the system automatically detects if it should try again. i'll leave the implementation up to u. hit me if u have any questions.

faceyspacey commented 7 years ago

by the way im gonna change it so the error is not caught. since the promise is returned, u gotta catch it in userland now.

theKashey commented 7 years ago

@faceyspacey - the question is - 'how'. Are you going to merge onLoad and onError to one promise-based property. And how it will work with synchronous node.js environment?

faceyspacey commented 7 years ago

u cant--because of the sync version. the sync version must be sync.

the callbacks by the way are fired right before the async .then/catch handlers are called.

i think it's pretty much the best we can do.

theKashey commented 7 years ago

So you`v got at least 3 ways to handle loading state - callback mostly for backend, promise only for preload, components. As for me - I prefer as stupid solutions, as they can be. And, for example, have been using only imports cos I did not find a way how to ensure that in both cases I load same file.