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

Collect context #12

Merged
merged 7 commits into from
Jan 29, 2020
Merged

Collect context #12

merged 7 commits into from
Jan 29, 2020

Conversation

bertho-zero
Copy link
Collaborator

@bertho-zero bertho-zero commented Jan 26, 2020

This PR allows adding several context updaters.
It also adds the withDefaults and withProps methods.

@bertho-zero
Copy link
Collaborator Author

The context updaters are all called before the first middleware: the params are no longer lost and we are no longer obliged to redeclare them at each level of hooks.

All middleware is grouped into a single chain and they are then executed around the deepest original method.

packages/hooks/src/base.ts Show resolved Hide resolved
@daffl
Copy link
Member

daffl commented Jan 28, 2020

Just one question, after that when you think it's good to go we can publish.

@daffl
Copy link
Member

daffl commented Jan 28, 2020

Oh also, is there a test that makes sure that walkOriginal works as expected? I believe something like

const sayHi = hooks(name => `Hi ${name}`, [
  async (context, next) => {
    console.log('context1');
    await next();
  }
]);


const exclamation = hooks(sayHi, [
  async (context, next) => {
    await next();
    context.result += '!';
  }
]);

Would now print context1 twice.

@bertho-zero
Copy link
Collaborator Author

It's already tested by existing tests, I just checked with your code and it works fine.

This is why there is this line:
https://github.com/bertho-zero/hooks/blob/f102cb2651efb98b3c06816d41aec735e1cb698c/packages/hooks/src/function.ts#L54

@bertho-zero
Copy link
Collaborator Author

bertho-zero commented Jan 28, 2020

I don't know if it's related but I have an issue with that:

class Service extends FeathersKnex {}

hooks(Service.prototype, [/* called */]);
hooks(Service.prototype, {
  method: [/* called */]
});

const service = new Service();

hooks(service, [/* NEVER called */]); // ISSUE HERE !
hooks(service, {
  method: [/* called */]
});

await service.method();

EDIT:
It was not related, the fix:

function walkProtoChain(obj, walker = Object.getOwnPropertyNames) {
  const proto = Object.getPrototypeOf(obj);
  const inherited = proto !== Object.prototype ? walkProtoChain(proto, walker) : [];

  return [...new Set(walker(obj).concat(inherited))];
}

// Before
for (const prop of walkProtoChain(service)) {
    module.exports[prop] = service[prop];
  }

// After
for (const prop of walkProtoChain(service)) {
    module.exports[prop] = typeof service[prop] === 'function'
      ? service[prop].bind(service)
      : service[prop];
  }

@bertho-zero
Copy link
Collaborator Author

I added the possibility to give default values to withParams:

// withParams(...params: Array<string | [string, any]>)
withParams('id', 'name', 'params')
withParams('id', 'name', ['params', {}])
it('creates context with default params', async () => {
  const fn = hooks(hello, {
    middleware: [
      async (ctx, next) => {
        assert.equal(ctx.name, 'Dave');
        assert.deepEqual(ctx.params, {});

        ctx.name = 'Changed';

        await next();
      }
    ],
    context: withParams('name', ['params', {}])
  });

  assert.equal(await fn('Dave'), 'Hello Changed');
});

@bertho-zero
Copy link
Collaborator Author

I don't see anything to add, you can merge as soon as you want.

@daffl
Copy link
Member

daffl commented Jan 29, 2020

I've been debating if there is a way to improve the syntax for setting params and other things by allowing to chain it:

const sayHi = hooks(name => `Hi ${name}`, [ hook1 ]).params('name').defaults({
  name: 'Dave'
});

This would still work on objects:

const o = {
  sayHi(name) { return `Hi ${name}` }
}

hooks(o, {
  sayHi: hooks([ hook1 ]).params('name').defaults({
    name: 'Dave'
  })
});

params.forEach((name, index) => {
context[name] = args[index];
export function withParams<T = any> (...params: Array<string | [string, any]>) {
return (self: any, _fn: any, args: any[], context: HookContext<T>) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can change the signature here but this does need to be documented.

@daffl daffl merged commit a556272 into feathersjs:master Jan 29, 2020
@daffl
Copy link
Member

daffl commented Jan 29, 2020

Thank you! I published these changes as v0.3.0. Even though this is backwards compatible, we still need some documentation for it though, otherwise nobody will know about it 🤔

@bertho-zero
Copy link
Collaborator Author

Missing on doc:

  • grouping of context updaters from all parts (Class + Class[method] + instance + instance[method]) and calling all before the first middleware
  • grouping of middleware from all parts (Class + Class[method] + instance + instance[method]) and calling it around the deepest original
  • changes in ContextUpdater signature: (self, fn, args, context)
  • withParams can contains default values (withParams('id', 'data', ['params', {}]))
  • new withProps

The problem is my ability to write correct explanatory text in English...

@bertho-zero bertho-zero mentioned this pull request Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants