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

Verify deprecated syntax outside the hook function #325

Merged
merged 1 commit into from
Jan 10, 2018
Merged

Verify deprecated syntax outside the hook function #325

merged 1 commit into from
Jan 10, 2018

Conversation

guillaumerxl
Copy link
Contributor

Hi,
I think it's better to set fieldNames outside the hook function. The current version makes fieldNames[0] to always be a string after fieldNames = fieldNames.slice(1);. So even if the developer uses the preventChanges(true, ...fieldNames) syntax, the hook will log the deprecated warning.
With this solution, the warning will be logged only if the developer is using the deprecated syntax.

Hi, I think it's better to set `fieldNames` outside the hook function. The current version makes `fieldNames[0]` to always be a string after `fieldNames = fieldNames.slice(1);`. So even if the developer uses the `preventChanges(true, ...fieldNames)` syntax, the hook will log the deprecated warning.
With this solution, the warning will be logged only if the developer is using the deprecated syntax.
@eddyystop
Copy link
Collaborator

I think the difference is

  1. The current code logs a message if the hook is coded with the deprecated signature.
  2. Your change logs a message if the hook is used with the deprecated signature.

I think its better to find errors asap. I would rather see a message during development when the server is started up, than depend on finding it in a log later on, possibly in production.

Also, the code handing the deprecated signature will be removed in the next major version, likely causing that signature to cause unexpected results or to crash the hook. So its important to identify what code needs to be reworked.

What are your thoughts?

@guillaumerxl
Copy link
Contributor Author

Hello,
it seems the current code logs a message even if the hook is not coded with the deprecated signature. This is what I'm experiencing.
But I agree with you about identifying the problem during the startup of the server.

So we just need to log the deprecated message outside of the hook function instead of logging it in each hook call?

@eddyystop
Copy link
Collaborator

I will look into this when I return from vacation next Wednesday.

@eddyystop eddyystop merged commit 2251ff6 into feathersjs-ecosystem:master Jan 10, 2018
@eddyystop
Copy link
Collaborator

I'll be adding a PR to log the deprecated message in the outer function.

Thanks for this.

@eddyystop
Copy link
Collaborator

Repo updated #329

It will be published to npm as 4.1..1 or 4.2.0.

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

Successfully merging this pull request may close these issues.

2 participants