have fp use meta name option instead of new symbol #44
Conversation
test/composite.test.js
Outdated
| @@ -11,5 +11,5 @@ test('anonymous function should be named composite.test', t => { | |||
| next() | |||
| }) | |||
|
|
|||
| t.is(fn[Symbol.for('fastify.display-name')], 'composite.test') | |||
| t.is(fn[Symbol.for('plugin-meta')].name, 'composite.test') | |||
There was a problem hiding this comment.
Can you name this fastify.plugin-meta?
There was a problem hiding this comment.
If the symbol is changed, this line in the fastify repo would also need to change, so changing the symbol could be considered a breaking change.
There was a problem hiding this comment.
Would this be considered a breaking change as I'd have to update fasitfy code as well? I'll push a commit asap 👍
test/composite.test.js
Outdated
| @@ -11,5 +11,5 @@ test('anonymous function should be named composite.test', t => { | |||
| next() | |||
| }) | |||
|
|
|||
| t.is(fn[Symbol.for('fastify.display-name')], 'composite.test') | |||
There was a problem hiding this comment.
Can you keep fastify.display-name as well? Otherwise it will be a breaking change, and I would not bump the major for this.
There was a problem hiding this comment.
Okay I'll keep it in even though it is redundant. Maybe we can make deprecation note for whenever fastify-plugin v2?
index.js
Outdated
| if (!options.name) { | ||
| let name = checkName(fn) | ||
| options.name = name | ||
| fn[Symbol.for('fastify.display-name')] = name |
There was a problem hiding this comment.
To retain full backward compatibility, this symbol should also be set when options.name exists. So this line should be moved after the if block.
|
Travis build needs restarting I think. I don't know whats causing some of the builds to complete and then other not. Anyways, this should be good to go now. I've included a new unit test that verifies a Once merged and deployed to npm I'll update the |
index.js
Outdated
| } | ||
|
|
||
| if (typeof options !== 'object' || Array.isArray(options) || options === null) { | ||
| throw new TypeError('The options object should be an object') | ||
| } | ||
|
|
||
| if (!options.name) { | ||
| let name = checkName(fn) | ||
| options.name = name |
There was a problem hiding this comment.
This could be simplified to
options.name = checkName(fn)
@nwoltman highlighted that
fastifyuses thefastify-pluginoptions.namemeta option property when adding plugins to theregistered-pluginlist. In conjunction with the newtoStringfeature (fastify/fastify#861) this PR includes changes that will makefpalways assign a function name to the meta option object instead of using an independent symbol (previously added via #37).Due note that this will change the fundamental behavior of
fastify-pluginas it will always assign theSymbol.for('plugin-meta')property to every function passed in.I kept support for using a version string as the 'options' parameter too.