WebReflection / workway

A general purpose, Web Worker driven, namespace exporter.
https://medium.com/@WebReflection/web-worker-driven-classes-and-methods-8a5a8e9bdb67
ISC License
184 stars 7 forks source link

Callbacks #1

Closed albertosantini closed 6 years ago

albertosantini commented 6 years ago

Congrats for Yet Another Awesome Project. :)

If you have a method like

function foo(message, callback)

you cannot use the usual approach (with foo in my namespace):

await my.foo("baz", data => console.log(data))

getting the error

TypeError: my.foo is not a function

due to the serialization rules (I suppose).

Here an example how to use in workway a method with a callback.

The code of the worker on Node.js side (my.js):

const util = require('util');

const workway = require('workway');

function foo(message, callback) {
    callback(null, message);
}

const fooPromisified = util.promisify(foo);

workway({
    fooPromisified
});

The code on the frontend side:

workway('node://my.js').then(async ({ namespace: my }) => {
    console.log(await my.fooPromisified("baz")); // prints "baz"
});

A few notes.

albertosantini commented 6 years ago
// with `callback(null, message);` prints "baz"
// with `callback(42, message);` prints undefined
// and catch part is never reached
// with throw new Error("custom error"); in the callback 
// the catch part prints Error: "custom error"

    my.fooPromisified("baz")
      .then(data => console.log(data))
      .catch(err => console.log(err));
WebReflection commented 6 years ago

arguments must be serializable, which is written in various places in the readme (comments) and also in the remote classes constrains: https://github.com/WebReflection/workway#the-remoteclass-convention

Is the promisified approach correct in the callback scenario?

You should return promises if the result is asynchronous so that's the best way

how to handle the error on the frontend side callback(42, message), because .then((err, data) => ... doesn't work, intending I get undefined values?

you should always set a worker.addEventListener('error', ...) to handle issues but yes, a rejected promise should quite possibly trigger rejection on the client side too.

I am closing this one since callbacks are clearly not possible and not meant to be used as arguments, since functions cannot be serialized as JSON.

WebReflection commented 6 years ago

You can throw an error in the callback (and it works nice), but how to handle the error on the frontend side callback(42, message), because .then((err, data) => ... doesn't work, intending I get undefined values?

I've just tried and everything works fine. It looks like you need to understand how util.promisify works, which is not how you imagine.

Given this callback:

function call(data, cb) {
  setTimeout(cb, 1, new Error('nope'));
}

and passing it through promisfy:

var p = require('util').promisify(call);

your promise won't return err, data but it will resolve on whatever data and it will throw errors if you pass the first argument:

p(123)
  .then(ok => console.log('yeah!'))
  .catch(err => console.error(err));

Your .then((err, data) => ... is a misunderstanding of the util.promosify utility as a whole, please read more about how to use it, 'cause you gonna have big issues if you expect the regular nodejs callback signature. The util is there to completely avoid that (err, data) thing.

albertosantini commented 6 years ago

I see. Thanks for the details.

Just in time, I need to double check promisify, because I am going to port a lot of api methods (in express realm) like

    app.post("/api/getOptimalPortfolio", jsonParser, (req, res) => {
        finance.portfolio.getOptimalPortfolio(req.body, (err, data) => {
            if (!err) {
                res.json(data);
            } else {
                res.json({
                    message: err
                });
            }
        });
    });
albertosantini commented 6 years ago

I am sorry if I review this issue.

But if I have a worker foo.js

const util = require('util');

const workway = require('workway');

function foo(message, callback) {
    callback(42); // forcing always an error
}

const fooPromisified = util.promisify(foo);

workway({
    fooPromisified
});

and a client

workway('node://foo.js').then(async ({worker, namespace:foo}) => {
  foo.fooPromisified("unused")
    .then(res => console.log("then", res))
    .catch(err => console.log("catch", err));
});

I would expect to see in the console catch 42 instead of then undefined.

As you showed with your example above, executing this snippet with Node.js, it returns catch 42.

const util = require('util');

function foo(message, callback) {
    callback(42); // forcing always an error
}

const fooPromisified = util.promisify(foo);

fooPromisified("unused")
    .then(res => console.log("then", res))
    .catch(err => console.log("catch", err));

Live demo: https://workway-node-bkytvieght.now.sh/ Live source: https://workway-node-bkytvieght.now.sh/_src

WebReflection commented 6 years ago

I think in some case I wasn't forwarding rejections errors properly. can you please try with latest and tell me if there's anything different?

albertosantini commented 6 years ago

Almost there: catch Error: "42" in workway, catch 42 in Node.js.

I suppose due to

https://github.com/WebReflection/workway/blob/master/index.js#L91

        if (message.hasOwnProperty('error'))
          promise.reject(new Error(message.error));

When the worker

https://github.com/WebReflection/workway/blob/master/worker.js#L86

      var rejected = function (error) { send(
        {error: error ? (error.message || error) : 'unknown'}
      ); };

Maybe simply promise.reject(message.error)?

WebReflection commented 6 years ago

@albertosantini so what do you expect instead ? an error cannot natively be serialized, so I either create one with the message, or I pass along whatever you rejected. If you reject a 42 who am I to decide that should be an Error instead?

WebReflection commented 6 years ago

If you reject a 42 who am I to decide that should be an Error instead?

or actually, what you are saying is that if you reject 42 you want that value to be passed along ?

albertosantini commented 6 years ago

I vote for this

or I pass along whatever you rejected

and this one

or actually, what you are saying is that if you reject 42 you want that value to be passed along ?

Basically I would like the same behavior I find in Node.js, described as catch 42. :)

WebReflection commented 6 years ago

Basically I would like the same behavior I find in Node.js

In node, the first argument, when it's an Error, it's an Error

fs.readFile('nope', (err, data) => console.log(err instanceof Error))
albertosantini commented 6 years ago

I am afraid I didn't express it correctly.

Intending Node.js style, where the callback follows the common error-first callback style, i.e. taking an (err, value) => ... callback as the last argument and the err is whatever.

albertosantini commented 6 years ago

But

workway('node://foo.js').then(async ({worker, namespace:foo}) => {
  foo.fooPromisified("unused")
    .then(res => console.log("then", res))
    .catch(err => console.log("catch", err.message));
});

err.message vs. the previous err calls the day (logging catch 42).

Really I don't know if it is better passing any or forcing Error.

What do you think?

Edit: Updated live demo - https://workway-node-ueyuwfgmhs.now.sh/

Edit2: On my side, it doesn't change anything because I have ...catch(err => ToastsComponent.update({ message: err.message || err }));

WebReflection commented 6 years ago

I think I've solved all the things with 0.5.2 ... please have a look when you have a chance.

albertosantini commented 6 years ago

With 0.5.2 I get err.message => "[object Object]"... The result is catch [object Object].

Well, giving a look at the changes I prefer the old simple code (and the results).

At the moment I published ConPA with 0.5.1 and it works ok.

WebReflection commented 6 years ago

Something ain't right, it shouldn't be like that, sorry

WebReflection commented 6 years ago

@albertosantini I cannot reproduce your scenario. Do you mind writing here your client and your server? the link you posted doesn't work anymore, but whatever I do I have the expected result, either a direct result of the failure, or an error with message and stack.

albertosantini commented 6 years ago

Here the details with workway 0.5.2:

client

workway('node://foo.js').then(async ({worker, namespace:foo}) => {
  foo.fooPromisified("unused")
    .then(res => console.log("then", res))
    .catch(err => console.log("catch", err));
});

worker

const util = require('util');

const workway = require('workway');

function foo(message, callback) {
    callback(42); // forcing always an error
}

const fooPromisified = util.promisify(foo);

workway({
    fooPromisified
});

Watching err in devtools:

columnNumber: 1180
fileName: "https://workway-node-iwpanvuqar.now.sh/js/3rd/workway.js"
lineNumber: 2
message: "[object Object]"
stack: "workway/</<@https: [cut]

In the console:

catch Error: "[object Object]"
workway https://workway-node-iwpanvuqar.now.sh/js/3rd/workway.js:2:1180
workway node.js:70:7
a https://workway-node-iwpanvuqar.now.sh/pocket.io/pocket.io.js:2:1099
a https://workway-node-iwpanvuqar.now.sh/pocket.io/pocket.io.js:2:1072
onmessage https://workway-node-iwpanvuqar.now.sh/pocket.io/pocket.io.js:2:406 index.js:4:19

Live demo: https://workway-node-iwpanvuqar.now.sh/ Live src: https://workway-node-iwpanvuqar.now.sh/_src

WebReflection commented 6 years ago

@albertosantini all you links give me 404, but I've created 3 files:

foo.html

<!doctype html>
<meta name="viewport" content="width=device-width,initial-scale=1">
<script>
if(!this.Promise)document.write('<script src="https://cdn.jsdelivr.net/npm/es6-promise@4/dist/es6-promise.auto.min.js"><'+'/script>');
if(!this.WeakMap)document.write('<script src="https://unpkg.com/poorlyfills@0.1.1/min.js"><'+'/script>');
try{new EventTarget}catch(e){document.write('<script src="https://unpkg.com/event-target@1.2.2/min.js"><'+'/script>')}
</script>
<script src="/workway.js"></script>
<script src="/pocket.io/pocket.io.js"></script>
<script src="/workway@node.js"></script>
<script>
workway('node://foo.js').then(async ({worker, namespace:foo}) => {
  foo.fooPromisified("unused")
    .then(res => console.log("then", res))
    .catch(err => console.log("catch", err));
});
</script>

foo.js in the same folder

var PORT = process.env.PORT || 3000;

var path = require('path');
var express = require('express');
var workway = require('../node').authorize(
  path.resolve(__dirname, 'workers')
);

var app = workway.app(express());
app.get('/', function (req, res) {
  res.writeHead(200, 'OK', {
    'Content-Type': 'text/html; charset=utf-8'
  });
  res.end(require('fs').readFileSync(path.resolve(__dirname, 'foo.html')));
});
app.get('/workway.js', function (req, res) {
  res.writeHead(200, 'OK', {
    'Content-Type': 'application/javascript; charset=utf-8'
  });
  res.end(require('fs').readFileSync(path.resolve(__dirname, '..', 'index.js')));
});
app.listen(PORT, () => {
  console.log('listening on http://localhost:' + PORT);
});

foo.js inside the workers folder

const util = require('util');

const workway = require('workway');

function foo(message, callback) {
    callback(42); // forcing always an error
}

const fooPromisified = util.promisify(foo);

workway({
    fooPromisified
});

All I see in console is catch 42.

Watch out !!!

Workers are very hard to be cleaned from cache ... are you cleaning up your cache when you try again?

Also, which browser are you using? I might try to clone and reproduce with the bundle version, maybe the problem is there.

WebReflection commented 6 years ago

@albertosantini I have created a standalone demo that does what I've described up there (since I've used this project test folder to create all that), it's called workway-demo (not the -node one, the -demo) https://github.com/WebReflection/workway/tree/master/examples

It has exactly this scenario. All you have to do is unzip it, go into that folder, run npm start and reach that page with any browser.

Open the console, and read catch 42, which I believe is the expected result.

What else am I missing?

albertosantini commented 6 years ago

TL;DR: workway-demo.zip works logging catch 42 (installed express before starting the app, because it is missing).

@albertosantini all you links give me 404,

Weirdly. here the last link works reproducing the problem. Never mind read ahead. :)

What else am I missing?

Basically my local demo (and the online one) was based on workway-node.zip.

To test the changes in terms of new releases, I modified it, changing the worker (workers/os.js), the client (js/index.js), and using pocket.io in index.html: the relevant code is the same compared with workway-demo.zip.

Then I updated workway with npm update (changing the package.json from 0.3.5 to 0.5.2), started the app and I got the error. Sigh. The code is the same. Double checked. The code is the same.

What is the culprit? :)

<script src="/js/3rd/hyperhtml.js"></script>

I missed that.

Mystery resolved.

Thanks a lot for fixing the error propagation and the patience to investigate these details.

I tested 0.5.2. in ConPA and it is ok.

Edit:

In my local demo, all it works with <script src="https://unpkg.com/workway"></script>

When I noticed the error I didn't test 0.5.2 with ConPA: it would have worked ok (because it would have loaded workway from node_modules). :)

WebReflection commented 6 years ago

@albertosantini FYI I have updated, and double tested, both demos in this folder: https://github.com/WebReflection/workway/tree/master/examples

There are also related READMEs

albertosantini commented 6 years ago

Thanks.

Tested and they work fine. :)