Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Depend on ES6-style promises instead of using Mithril deferreds #800

Closed
dead-claudia opened this issue Sep 28, 2015 · 22 comments
Closed

Depend on ES6-style promises instead of using Mithril deferreds #800

dead-claudia opened this issue Sep 28, 2015 · 22 comments
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code

Comments

@dead-claudia
Copy link
Member

This has been discussed on Gitter multiple times, and @lhorie himself initially came up with the idea. Currently, the deferred implementation makes up 180 lines of code, incl. comments, and converting the rest of this to use ES6-style promises would also simplify m.request() some. There's also smaller implementations than Mithril's as well, such as promiscuous, which is even Promises/A+ compliant and has a Promise.all implementation as well.

Basically, this is seeing if the left side may be deprecated in favor of the right side, with Mithril internally changing to use the right:

Deprecated New
m.deferred() new Promise()
m.sync() Promise.all()

What do you all think?

@dead-claudia
Copy link
Member Author

One open question, though, is how to deal with m.prop() in this light.

@StephanHoyer
Copy link
Member

I rarely use m.prop. Can you explain what exactly is the problem with it? Do you mean this?

@dead-claudia
Copy link
Member Author

@StephanHoyer Yes, particularly where it wraps Mithril promises.

Would wrapping it to synchronously change the 'prop' value when called, but asynchronously changing it when the promise resolves, and asynchronously returning the value be a good idea? It's open to race conditions, though, in that you shouldn't change it between the tick of creation and the tick when then is called from it, as it could conflict with m.prop's handling. The reason why calling changes it synchronously is because it would become nondeterministic and open to implicit race conditions if it wasn't.

Would something like this work? Where it accepts a promise and returns a thenable?

function createPromiseProp(promise) {
  var ret = m.prop()
  var p = promise.then(ret)
  ret.then = p.then.bind(p)
  ret.catch = p.then.bind(p, undefined)
  return ret
}

@barneycarroll
Copy link
Member

Looks OK to me:

const gimme5 = () =>
  new Promise( resolve => resolve( 5 ) )

const pp = m.prop( gimme5() ).then( x => x * 5 )

console.log( pp() )

setTimeout( () => 
  console.log( pp() )
)

EDIT: I can imagine a few edge cases relating to generic language-level expectations with different Promise implementations - @IMPinball is definitely right in referring to the output as a 'thennable'. You can merge an object into a function but not vice versa, so this will always be an extended m.prop rather than an extended Promise.

@dead-claudia
Copy link
Member Author

m.prop would have to turn into this, though, to avoid a breaking change:

m.prop = function (value) {
  // Most promises
  if (isPromise(value)) return createPromiseProp(value)

  // Older Mithril deferreds, deprecate, but keep compatibility
  // in meantime
  if (value != null && isPromise(value.promise)) {
    return createPromiseProp(value.promise)
  }

  return gettersetter(value)
}

@dead-claudia
Copy link
Member Author

@barneycarroll Actually, it already kind of does it. It's just a little too infectious.

@veggiemonk
Copy link
Contributor

That would be really neat!!! 👍

@dead-claudia dead-claudia added the Type: Breaking Change For any feature request or suggestion that could reasonably break existing code label Dec 15, 2015
@Satyam
Copy link

Satyam commented Dec 16, 2015

Mithril contains 4 major parts: Rendering, Routing, Request and Promises.

What I would suggest is creating a separate src folder and have each of those major parts into separate files and then with suitable wrappers merge them to create the Mithril distribution as it now stands, without any breaking changes and provide the other parts as separate Npm modules under a dist folder so as to pack them with Browserify or WebPack.

Suitable documentation could explain how to wrap your async request module with start/endComputation so as to trigger a redraw.

@dead-claudia
Copy link
Member Author

@Satyam The general consensus is that Webpack and Browserify are both no-gos, because their boilerplate amounts to half of Mithril's existing size. Most likely, we'll use something similar to Lodash, or some other homegrown solution. And as for wrapping another Ajax helper, Mithril already has that as an example in its documentation.

@barneycarroll
Copy link
Member

Considering Mithril is so small and the dependency structure so simple, a simple concatenation script might do…?

@Satyam
Copy link

Satyam commented Dec 16, 2015

@barneycarroll : Indeed, that was the idea, Sorry for my bad description. @isiahmeadows : I meant the separate modules to be made NPM compatible, so that they can be packed, however I failed to make it clear that the complete mithril.js is to be built from concatenation of the separate parts and then wrapped in the UMD header and footer.

They might as well all use UMD wrappers but I see the separate parts more likely to be packed together with other modules such as, say, Axios for http request or the Babel polyfill for Promises or whatever else you want to add, rather than included via a script tag.

So, the original source files for the parts would be headerless, no CommonJS nor UMD wrapper. The build process would wrap each part into a separate NPM module in the dist folder and then all of them concatenated and wrapped in the current UMD header and placed in its current location.

Alternatively, all the source files might have plain CommonJS wrapper which could be stripped out to produce the headerless files to be concatenated and then wrapped in the UMD wrapper. Some marking in the source files, some special comment line, would mark the cut point. This would also allow to cut out, besides the CommonJS header, any utility functions that would otherwise get repeated when the files get concatenated.

I prefer simple concatenation rather than cutting out parts and then merging. The build process should also take care of providing each separate part with its corresponding copy of the currently shared utility functions while, at the same time, having only one copy on the full version.

Test files would also need to be split into separate test files plus the current ones for the whole Mithril.

Also, once separate, the parts that are not core to Mithril (HTTP request and Promises and, perhaps in the future, routing) should have its development halted. Only critical errors. With so many alternatives available for their functionality, it makes no sense to devote any resources on them.

@pygy
Copy link
Member

pygy commented Dec 17, 2015

It's been discussed elsewhere, but, did you guys check out Rollup?

/ducks ;-)

edit: http://rollupjs.org/

@Satyam
Copy link

Satyam commented Dec 17, 2015

A simple way to make those wrappers would be something like this:

/*#isString.js#*/
// any number of other dependencies
/*#router.js#*/
module.exports = mroute;

The /*# #*/ comments are the 'include' directive for the source files. IN this case, we include one of the small utility functions and then the router code. For the complete Mithril, the wrapper would have the UMD header and then a bunch of these 'include' directives.

;(function (global, factory) { // eslint-disable-line
    "use strict"
  // rest of UMD stuff
})(this, function (window, undefined) { // eslint-disable-line
    "use strict"
/*#isString.js#*/
// all the rest of the utility functions
/*#renderer#*/
global.m = m;
/*#request.js#*/
m.request = mrequest; //mrequest doesn't really exist right now
// and so on and so forth
 return m;
});

A Grunt plugin would need to be developed, unless there is already one for such a thing, which I haven't checked.

Alternatively, the same thing could be accomplished by concatenating bits and pieces but it would really make it hard to understand the complete code with just a bunch of disconnected fragments, connected by the gruntfile.

It would also be good if the individual parts have valid syntax so linters and code highlighters don't get mad.

Regarding tests, I would suggest that the current set of tests should be able to check both the complete Mithril.js code and the Browserified bundle of the separate parts. Since the utility functions are private to each module so that even when loaded all at once one module would not see the utility functions of the other modules, I guess this would make for a good enough test.

@dead-claudia
Copy link
Member Author

@Satyam The best solution would have to be written from scratch.

@dead-claudia
Copy link
Member Author

For what it's worth, though, I'm thinking of making a mini module on npm
(easier to test, independent versioning) for this particular use case, as
it's relatively unusual.

On Thu, Dec 17, 2015, 03:32 Daniel Barreiro [email protected]
wrote:

A simple way to make those wrappers would be something like this:

/#isString.js#/
// any number of other dependencies
/#router.js#/
module.exports = mroute;

The /# #/ comments are the 'include' directive for the source files. IN
this case, we include one of the small utility functions and then the
router code. For the complete Mithril, the wrapper would have the UMD
header and then a bunch of these 'include' directives.

;(function (global, factory) { // eslint-disable-line
"use strict"
// rest of UMD stuff
})(this, function (window, undefined) { // eslint-disable-line
"use strict"
/#isString.js#/
// all the rest of the utility functions
/#renderer#/
global.m = m;
/#request.js#/
m.request = mrequest; //mrequest doesn't really exist right now
// and so on and so forth
return m;
});

A Grunt plugin would need to be developed, unless there is already one for
such a thing, which I haven't checked.

Alternatively, the same thing could be accomplished by concatenating bits
and pieces but it would really make it hard to understand the complete code
with just a bunch of disconnected fragments, connected by the gruntfile.

It would also be good if the individual parts have valid syntax so linters
and code highlighters don't get mad.

Regarding tests, I would suggest that the current set of tests should be
able to check both the complete Mithril.js code and the Browserified bundle
of the separate parts. Since the utility functions are private to each
module so that even when loaded all at once one module would not see the
utility functions of the other modules, I guess this would make for a good
enough test.


Reply to this email directly or view it on GitHub
#800 (comment).

@pygy
Copy link
Member

pygy commented Dec 18, 2015

Regarding promises, I'm definitely for using ES6 promises. You can keep a m.deferred stub for backwards compatibility in most cases.

    function deferred() {       
        var prm = {}
        prm.promise = propify(new mPromise(function(R,r){
            prm.resolve = R
            prm.reject = r
        }))
        return prm
    }

where mPromise defaults to the global Promise object, but can be set to something else.

The same goes for m.sync.

I'm not sure the current implementation of propify is correct, though (at least, not according to what I had in mind when I wrote it). It doesn't seem to be contagious like it once was: foo = propify(promise).then(...) foo should be propified too (a different prop than propify(promise)).

Potentially synchronous promises can be toxic for some use cases (I've tried to build an async Mithril module loader that handles dependencies chains, and I was running into race conditions that were not present with A+ Promises). It makes promise composition much harder.

@dead-claudia
Copy link
Member Author

I'm intentionally delaying until the npm version is bumped before converting to native Promises.

@pygy
Copy link
Member

pygy commented Dec 19, 2015

Will you keep a m.deferred stub? The executor pattern makes the trivial case easy, but when you want something fancy, you often end up emulating the deferred API

BTW, the sooner you adopt ES6 it the better. Between v0.2.0 and the current next HEAD, we went from 62 to 409 failures out of 872 Promise/A+ test cases. It even breaks the test suite if m.deferred.onerror isn't disabled (-:.

If you didn't keep a close eye on the test suite, it's not surprising that there are regressions, promises are tricky to get right.

Also, it may be nice to restore the infectious nature of propify, like this:

function propify(promise, initialValue) {
    prop = m.prop(initialValue)
    promise.then(prop)

    prop.then = function(res, rej) {return propify(promise.then(res, rej))}
    prop.catch = function(rej) {return propify(promise.then(null, rej))}

    // keeping this mostly as is, not sure about the use case
    prop.finally = function (callback) { 
        return propify(promise.then(function (value) {
            return simpleResolve(callback(), function () {
                return value
            })
        }, function (reason) {
            return simpleResolve(callback(), function () {
                throw reason
            })
        }))
    }
    return prop
}

Passing prop() explicitly like here is not needed (at least not if your implementation is A+ correct).

@dead-claudia
Copy link
Member Author

@pygy

Will you keep a m.deferred stub? The executor pattern makes the trivial case easy, but when you want something fancy, you often end up emulating the deferred API

Yes, but it be in a separate file and include an async wrapper so it can be used in core. I will also include m.deps.promise() to set the internal Promise constructor used in core, defaulting to the system version. I will also include very specific specs required.

BTW, the sooner you adopt ES6 it the better. Between v0.2.0 and the current next HEAD, we went from 62 to 409 failures out of 872 Promise/A+ test cases. It even breaks the test suite if m.deferred.onerror isn't disabled (-:.

If you didn't keep a close eye on the test suite, it's not surprising that there are regressions, promises are tricky to get right.

Mithril's m.deferred intentionally deviated from Promises/A+ a while back (at least a year ago), for performance reasons (which are relatively moot now) and type safety (which IMHO is not as big as it sounds). That intentional deviation still remains the case, and if we realign, it'll break a lot of things in userland, and require some things in core to be rewritten. Or in other words, it's more trouble than it's worth.

Also, it may be nice to restore the infectious nature of propify, like this:

That's not going to happen. Especially with m.deferred and m.sync being deprecated the release after next (@lhorie is planning on doing one probably today or tomorrow). m.prop() is going to remain an agnostic thenable, depending on the implementation of the promise it's wrapping. Also, the extra layer of indirection is only going to increase memory and slow things down pointlessly.

The reason why is because Mithril shouldn't be reinventing yet another wheel that when incompatibility arises, is known to cause problems. (Read: lesson learned)

Matter of fact, most major frameworks with deferreds either depend on a third-party promise implementation that supports Promises/A+, like Ember or borrow one that does without changing the core implementation, like Angular (which only removed features). ExtJS pulled their implementation from DeftJS with mostly name changes, but made sure to keep it spec-compliant the whole time.

@mikegleasonjr
Copy link

Personally I would ditch any Promise code in Mithril and let the client link its own library if he want to use it on his side! I just saw that a lot has been talked about after opening #947. Sorry for that, should I bring my issue in a comment here?

@dead-claudia
Copy link
Member Author

@mikegleasonjr BTW, @lhorie is planning on ditching m.deferred() and m.sync() in his rewrite he's working on. And yes, this is the right bug to talk about removing Mithril deferreds in favor of Promises.

@lhorie
Copy link
Member

lhorie commented Jun 1, 2016

Rewrite's implementation follows ES6 API and is A+ compliant

m.request's factory allows depending on different Promise implementations so it's possible to not use Mithrils's implementation, if that's what's desired

@lhorie lhorie closed this as completed Jun 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code
Projects
None yet
Development

No branches or pull requests

8 participants