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

Question about Promise adaptor #2983

Closed
PaulMaly opened this issue May 29, 2017 · 27 comments
Closed

Question about Promise adaptor #2983

PaulMaly opened this issue May 29, 2017 · 27 comments

Comments

@PaulMaly
Copy link

Hi, all! I'm using Promise adaptor and basically, it's a nice tool to set some async values right into Ractive data. But now I want to use built-in Promise returned by ractive.set() method.

The problem is that this promise resolved instantly, before Promise adaptor's resolved real value. But I want to do something like this:

ractive.set('items', api.get('items')).then(() => { /* do something with real values */ })

Is any ideas how to rewrite Promise adaptor to support this behaviour?

@evs-chris
Copy link
Contributor

Unfortunately there's not really a way for adaptors to do that. The Promise returned from set and friends resolves when the DOM is settled after any transitions, which is on the next tick when no transitions are involved. The one thing we could do would be to resolve the set Promise with the value that was set, which would then defer it until both transitions are complete and the given Promise resolves. That seems like it could be a bit of a trap though, if you're expecting maybe transitions on the second resolution to also be complete.

I think the best solution here would be a monkey-patch:

const realSet = Ractive.prototype.set;
Ractive.prototype.set = function(keypath, value, options) {
  if (value instanceof Promise) return Promise.all([value, realSet.call(this, keypath, value, options)]).then(function(arr) { return arr[1]; });
  return realSet.call(this, keypath, value, options);
}

Most of the API methods on the Ractive prototype are just sugaring internal functions, so it's generally pretty safe to add more sugar on top.

@PaulMaly
Copy link
Author

PaulMaly commented May 30, 2017

@evs-chris, hi and thanks for the answer! It's little bit sad that adaptors can't be used for this purpose. How to you think, maybe this feature need to be included in Ractive itself? I suppose that because promises are very useful and widespread thing right now. Many-many libraries and modules supports it and it can improve Ractive's set() capability.

One more, it really strange to me, that ractive.set() resolve a promise without value which was set. I mean that ractive.set().then() doesn't have any arguments. I think more useful something like this:

ractive.set('item', 'My item').then(console.log) // 'My item' printed in console

@fskreuz
Copy link
Contributor

fskreuz commented May 30, 2017

One more, it really strange to me, that ractive.set() resolve a promise without value which was set.

This is because the promise returned by ractive.set() isn't related to the set operation at all. It's a promise that resolves after the transitions it caused resolves. If any value were to be provided as the resolved value, it would be related to the transition, and not the set operation.

I think more useful something like this:
ractive.set('item', 'My item').then(console.log) // 'My item' printed in console

The real problem is that there's actually two ractive.sets happening - one is setting the promise as the value (your code), and the other is setting the real value after the promise is resolved (adaptor code). What you're looking for to hook on to is the second set call, which is trapped inside the adaptor. Also, this is an implementation detail of that adaptor.

But then, that's what adaptors are for. If this feature needs to exist, it would be best implemented on that adaptor. I would imagine that set monkey patch would be done in the adaptor, and that your code and Ractive would be oblivious of its existence.

@PaulMaly
Copy link
Author

@fskreuz hi and thank you too!

This is because the promise returned by ractive.set() isn't related to the set operation at all. It's a promise that resolves after the transitions it caused resolves.

Yea, I know, but it's strange too because if I do set operation and this method returns some promise I undoubtedly expect that this promise relates to this operation. I can happen after a transition, it's ok, but promise itself need to be related to set operation. It's more obvious behaviour I think.

Additional question, where can I get changelog for 0.9.0 version? I just install it from npm and all my project became a broken. Whether there are some breaking changes in this version?

@fskreuz
Copy link
Contributor

fskreuz commented May 30, 2017

There's a changelog file in the repo. I think we should add it to the package along with the readme next time for an easy lookup.

@evs-chris
Copy link
Contributor

There's a migration page in the docs with known breaking changes: https://ractive.js.org/get-started/migrating

Let us know if there's something we missed!

@PaulMaly
Copy link
Author

Thanks, guys! Unfortunately, I see not too encouraging trends in latest versions. In my opinion, of course, but it's completely up to you.

Anyway, I've resolved most of the issues except one: when I build Reactive and other modules, using Webpack 2 I got a strange error on the client:

Slider.js:14 Uncaught TypeError: o.extend is not a function

Debug in Chrome shows me that it's really true. Ractive itself looks like that:

Reactive: Object

	default:function t(e)

 		defaults { <here regular Ractive things like extend etc.> }

 	easing:Object

	transitions:Object

My previous version is 0.8.14, webpack config the same. I guess why easing and transitions object was included to Ractive too:

Ractive.transitions = require('./animations/transitions');
Ractive.easing = require('./animations/easing');

But why Ractive become this strange object I don't understand.

@sabob
Copy link
Contributor

sabob commented May 30, 2017

@PaulMaly what are the discouraging trends? Too many features, not enough features?

@evs-chris
Copy link
Contributor

0.9 includes an ES module bundle, and I think it's not getting along with webpack. I'm looking into that, though it may take me a bit as I've never used webpack before.

@evs-chris
Copy link
Contributor

Regarding the webpack issue, I've confirmed that everything works out using the ractive ES module bundle with something like:

import Ractive from 'ractive';

const Component = Ractive.extend({
  template: 'Hello'
});

new Component({
  target: '#target'
});

is working. I think the issue is probably with the ractive-component-loader, if that's what's being used. I'll see if I can track that down and see what it's doing.

@PaulMaly
Copy link
Author

PaulMaly commented May 30, 2017

@sabob Actually, isn't a lot to devote too much time to discuss it. I see that Ractive moves forward a little bit quicker than earlier.

I don't like directives-based approach strengthening. I didn't like directives in Angular, I hate that in Vue. I hope that this approach won't become the cornerstone of Ractive. I don't like reduction of browsers support. I don't like some complications in Ractive's concepts. I think that Adaptors is a very useful thing but too weak than should be. I think that server-side capabilities also could be more advanced. I really like Ractive, I wrote much code with Backbone and Angular before. I tried Riot, React and Vue, but Ractive still my favourite tool. Yes, Ractive not so popular, not so big community and not a lot of modules for it. Ractive slower than most of these tools and have a big size. But Ractive always was most simple to understand and use tool. Minimum complexity, maximum usability, you know.

@evs-chris I don't use ractive-component-loader. I just use Webpack to support CommonJS modules on the client and some other applied things like minification, etc.

using the ractive ES module

Unfortunately, I can't use ES modules, because they are not supported by NodeJS. I use isomorphic approach and for me, it's very important. The main thing that previous version works fine on the same config, but a new version is not. Seems that an issue.

@PaulMaly
Copy link
Author

@fskreuz

But then, that's what adaptors are for. If this feature needs to exist, it would be best implemented on that adaptor. I would imagine that set monkey patch would be done in the adaptor, and that your code and Ractive would be oblivious of its existence.

I use CommonJS modules on client and server sides. So, I don't have any global Ractive in my Promise adaptor module. It just uses ractive instance argument of wrap function.

@fskreuz
Copy link
Contributor

fskreuz commented May 30, 2017

This is the just the kind of feedback we need. 👍 I agree that Ractive can be simpler. I have my own opinions, but we'll have to save these up for 2.0. 🎉 For now, we need it to be stable and bug-free for a 1.0 release.

@evs-chris
Copy link
Contributor

@PaulMaly attempting to address some concerns:

  • The directives are mostly the same ones that have been in place since < 0.7, they just restructured a bit for parsing clarity (both code and coder). The only additional directive in 0.9 is to control event delegation in a fine-grained way, but you can safely turn off delegation completely and ignore it. The class- and style- directives added in 0.8 are mostly for performance and developer convenience - again, not required as style and class still work.
  • IE8 is the only browser that got dropped. To support IE9+, you just need to include the polyfills file ahead of ractive. IE8 was getting very cumbersome to support, and in fact, it didn't fully work with 0.7. IE9 is much less cumbersome, though it is definitely still an IE 😄.
  • There's actually an open issue Adaptors on edge - Discussion #2512 to address issues with adaptors. They're queued for an API overhaul that will give them more control than they have now. Swing by that issue when you get some time and add what you'd like to be able to support over there.
  • As of 0.9, Ractive is not much slower than vue, react, or angular in some metrics and is faster in others based on the frameworks benchmark. That particular benchmark is a bit of the worst case in terms of style and layout penalties. Bigger is on the list of things to address for 1.0.
  • I think the worst change as far as adding complexity was switching to expressions for events and the introduction of @this. To some extent, I regret that change, but I also see it as necessary for resolving the weird state of event listeners that had come to be. There are no more planned for things like that 😄.

Can you exapnd a bit on what you'd like to see more in regards to server-side capabilities?

As far as ES modules, the one shipped with ractive now should only be picked up by systems that can use ES modules, like webpack and rollup. The UMD bundle is still the default, so it should be fine for both node and browser environments. @mpowell-atomic also had an issue, but got it resolved by deleting ractive/ractive.mjs from node_modules. Again, I suspect it's a loader issue because my test with webpack worked surprisingly well and it was definitely using ractive.mjs.

As for the adaptor needing access to Ractive, that seems like something that should be accessible from an instance, probably with instance.constructor.Ractive. I think instance.constructor.Parent should also be available for extended extensions. I'll flag that up for 0.9.1.

@fskreuz
Copy link
Contributor

fskreuz commented May 31, 2017

I use CommonJS modules on client and server sides. So, I don't have any global Ractive in my Promise adaptor module. It just uses ractive instance argument of wrap function.

The way I understand module management in Node is that once a module is loaded, anything else requiring it will refer to that loaded module and not some clone/duplicate/separate instance. Assuming everything loads the same adaptor module and Ractive, modifying the Ractive prototype from the adaptor module should modify it once and permanently... or at least that's the theory. I may be wrong tho :D

const Ractive = require('ractive');

// Once this module gets required somewhere, monkey-patch Ractive.prototype.set
const realSet = Ractive.prototype.set;
Ractive.prototype.set = function(keypath, value, options) {
  if (value instanceof Promise) return Promise.all([value, realSet.call(this, keypath, value, options)]).then(function(arr) { return arr[1]; });
  return realSet.call(this, keypath, value, options);
}

module.exports = function MyAdaptor(){ ... }

@PaulMaly
Copy link
Author

PaulMaly commented Jun 1, 2017

@evs-chris

The directives are mostly the same ones that have been in place since < 0.7, they just restructured a bit for parsing clarity (both code and coder).

I don't remember when exactly new transition syntax was implemented but this syntax generates many additional directives. In my opinion, an old syntax of transitions was more laconic.

They're queued for an API overhaul that will give them more control than they have now.

Wow, it's a great news! Thanks!

As of 0.9, Ractive is not much slower than vue, react, or angular in some metrics and is faster in others based on the frameworks benchmark.

It would be very awesome! My comment based on this publications cycle: http://www.stefankrause.net/wp/?p=431
But seems that latest benchmark doesn't contain 0.9.0 version of Ractive. So, I believe that soon we'll see the new version in the next benchmark and will be surprised with results. )))

I think the worst change as far as adding complexity was switching to expressions for events and the introduction of @this.

Yes, that's one of the things what I'm talking about.

@fskreuz

The way I understand module management in Node is that once a module is loaded,

Yes, modules are caching, but it's a strange architectural proposal. Right now promise adaptor depends only on arguments passed to it, and it's good practice. My guess, it would be much more preferable if it will be available out-of-box.

@evs-chris @fskreuz

Can you exapnd a bit on what you'd like to see more in regards to server-side capabilities?

The main problem on the server side is not "How we render an app?", Ractive does this well, but "When we need to render an app and send the response?" If we try to create our components isolated from another code, components need to make async operations inside of it. On the client, it's not a problem, because of client fully asynchronous, but server response is not, we need to decide a point when we can send fully rendered app with all data, after all async operations inside of all components.

I try to do something like this:

// app.js

const Ractive = require('ractive');

const app = new Ractive({
        target: '#app',
        enhance: true,
        template: require('./templates/parsed/app'),
        components: {
		subclass: require('./components/subclass')
	},
	data: function() {
		return {};
	},
        onconfig: function() {
                this.promises = [];
        },
        oninit: function() {
	        const $$ = this;
	        Promise.all($$.promises.map(p => p.catch(e => e)))
			    .then(res =>$$.fire('ready', null, res))
			    .catch(err => $$.fire('ready', err));
        }
});

// subclass.js

const Ractive = require('ractive');
const api = require('../api');

module.exports = Ractive.extend({
    enhance: true,
    css: '@import "/styles/subclass.css?' + Date.now() + '";',
    template: require('../templates/parsed/subclass'),
    adapt: [
        require('../adaptors/promise')
    ],
    data: function() {
        return {
            items: []
        };
    },
    onconfig: function() {

        const items = this.set('items', api.get('items'));

        this.root.promises.push(items);
    }
});

So, an app "ready" event will be triggered only when all promises from all components, which add themselves to be rendered on the server (some components or some async operations can be not added, if it's not necessary), will be resolved or rejected (when we'll try to resolve them on client).

But, it's not working right now, because set operation promise resolving at the wrong time with wrong value. To do that, we need to resolve this promise only then the actual value will be set even if it's an another promise. And this promise needs to be resolved with the actual value, not with undefined or transition things.

@sabob
Copy link
Contributor

sabob commented Jun 1, 2017

I think the worst change as far as adding complexity was switching to expressions for events and the introduction of @this.

Difficult to balance simplicity vs expressiveness...

Not sure if it is too late to revert some complexity but a "Best Practice" guide might be useful to show idiomatic Ractive template syntax.

@PaulMaly
Copy link
Author

PaulMaly commented Jun 1, 2017

@sabob "Best Practice" is a great idea! Most of the existing frameworks provide only "hello world" things, but Ractive always had an interactive tutorial and I think "Best Practice" which solves main development issues, will be very useful to start with Ractive.

@evs-chris
Copy link
Contributor

As of 0.9.1, you can access the Ractive constructor from any component constructor, so the promise adaptor, or whatever can monkey patch methods as needed.

@PaulMaly
Copy link
Author

As of 0.9, Ractive is not much slower than vue, react, or angular in some metrics and is faster in others based on the frameworks benchmark. That particular benchmark is a bit of the worst case in terms of style and layout penalties. Bigger is on the list of things to address for 1.0.

So-so: http://www.stefankrause.net/wp/?p=454

@evs-chris
Copy link
Contributor

Oddly enough, it seems startup time is our worst metric now. I think using the ractive-bin-loader in the webpack config would result in a significant drop (maybe half) in startup time because parsing isn't exactly cheap.

@fskreuz
Copy link
Contributor

fskreuz commented Nov 22, 2017

Fun observation: Ractive's implementation appears to be purely runtime... while the benchmark is against compiled/half-compiled implementations. Not too bad really. :P

btw, i'd avoid hijacking the thread. :P

@PaulMaly
Copy link
Author

I use fully pre-compiled templates during packaging via Webpack. How do you think how much acceleration it gives as a result?

@PaulMaly
Copy link
Author

And one more point about this case:

Thanks, guys! Unfortunately, I see not too encouraging trends in latest versions. In my opinion, of course, but it's completely up to you.
Anyway, I've resolved most of the issues except one: when I build Reactive and other modules, using Webpack 2 I got a strange error on the client:
Slider.js:14 Uncaught TypeError: o.extend is not a function

Unfortunately, it's still happening. And I don't know what I can do with this. CommonJS still more convenient for me to include dependencies. On the server-side everything OK, but on the client, I see this object instead Ractive constructor:

const Ractive = require('ractive');
console.log(Ractive);
// Ractive:
    default:
        ƒ Ractive( options )
        __esModule: true
        __proto__: Object

Maybe you know what does it mean? Maybe it will help you:
babel/babel#5079
https://stackoverflow.com/questions/41289200/output-an-es-module-using-webpack

Can we somehow prevent this problem? I will be very grateful if you provide a solution.

@evs-chris
Copy link
Contributor

The issue is that [webpack doesn't support default exports in a configurable way] (webpack/webpack#4742), so we may need to drop the module field from our package manifest if we need to support commonjs in bundlers. I think that's not a particularly good solution though. You can probably set up an alias or something like it to make sure you get the cjs build of ractive rather than the es build.

@PaulMaly
Copy link
Author

PaulMaly commented Nov 22, 2017

Ok, but why I don't get this issue using other libs? Most of them work well with both - CommonJS and ES6.

You can probably set up an alias or something like it to make sure you get the cjs build of ractive rather than the es build.

It seems I did not fully understand this point, could you please provide more details?

@PaulMaly
Copy link
Author

The issue is that [webpack doesn't support default exports in a configurable way] (webpack/webpack#4742),

Seems, it can be fixed via Webpack config. FYI:

resolve: {
       mainFields: ['browser', 'main']
},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants