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

add option to encapsulate #199

Merged
merged 6 commits into from
Oct 13, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,29 @@ module.exports = fp(plugin, {
})
```

#### Encapsulate
You can optionally keep the plugin encapsulated.
dwickern marked this conversation as resolved.
Show resolved Hide resolved
```js
const fp = require('fastify-plugin')

function plugin (fastify, opts, next) {
// the decorator is not accessible outside this plugin
fastify.decorate('util', function() {})
next()
}

module.exports = fp(plugin, {
name: 'my-encapsulated-plugin',
fastify: '4.x',
decorators: {
fastify: ['plugin1', 'plugin2'],
reply: ['compress']
},
dependencies: ['plugin1-name', 'plugin2-name'],
encapsulate: true
})
```

#### Bundlers and Typescript
`fastify-plugin` adds a `.default` and `[name]` property to the passed in function.
The type definition would have to be updated to leverage this.
Expand Down
3 changes: 2 additions & 1 deletion plugin.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ export interface PluginMetadata {
request?: (string | symbol)[]
},
/** The plugin dependencies */
dependencies?: string[]
dependencies?: string[],
encapsulate?: boolean
}

// Exporting PluginOptions for backward compatibility after renaming it to PluginMetadata
Expand Down
7 changes: 2 additions & 5 deletions plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,6 @@ function plugin (fn, options = {}) {
)
}

fn[Symbol.for('skip-override')] = true

const pluginName = (options && options.name) || checkName(fn)

if (typeof options === 'string') {
options = {
fastify: options
Expand All @@ -42,9 +38,10 @@ function plugin (fn, options = {}) {

if (!options.name) {
autoName = true
options.name = pluginName + '-auto-' + count++
options.name = checkName(fn) + '-auto-' + count++
}

fn[Symbol.for('skip-override')] = !options.encapsulate
jsumners marked this conversation as resolved.
Show resolved Hide resolved
fn[Symbol.for('fastify.display-name')] = options.name
fn[Symbol.for('plugin-meta')] = options

Expand Down
6 changes: 4 additions & 2 deletions plugin.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ expectAssignable<FastifyPluginCallback>(fp(pluginCallback, {
reply: [ '', testSymbol ],
request: [ '', testSymbol ]
},
dependencies: [ '' ]
dependencies: [ '' ],
encapsulate: true
}))

const pluginCallbackWithOptions: FastifyPluginCallback<Options> = (fastify, options, next) => {
Expand Down Expand Up @@ -66,7 +67,8 @@ expectAssignable<FastifyPluginAsync>(fp(pluginAsync, {
reply: [ '', testSymbol ],
request: [ '', testSymbol ]
},
dependencies: [ '' ]
dependencies: [ '' ],
encapsulate: true
}))

const pluginAsyncWithOptions: FastifyPluginAsync<Options> = async (fastify, options) => {
Expand Down
35 changes: 35 additions & 0 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,3 +237,38 @@ test('should check fastify dependency graph - decorateReply', t => {
t.equal(err.message, "The decorator 'plugin2' required by 'test' is not present in Reply")
})
})

test('should accept an option to encapsulate', t => {
t.plan(4)
const fastify = Fastify()

fastify.register(fp((fastify, opts, next) => {
fastify.decorate('accessible', true)
next()
}, {
name: 'accessible-plugin'
}))

fastify.register(fp((fastify, opts, next) => {
fastify.decorate('alsoAccessible', true)
next()
}, {
name: 'accessible-plugin2',
encapsulate: false
}))

fastify.register(fp((fastify, opts, next) => {
fastify.decorate('encapsulated', true)
next()
}, {
name: 'encapsulated-plugin',
encapsulate: true
Copy link
Member

Choose a reason for hiding this comment

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

We need a test that verifies what the docs say: does it run all the validations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you say the 1 test is enough? I'd rather avoid repeating the whole test suite

Copy link
Member

@Eomm Eomm Sep 24, 2022

Choose a reason for hiding this comment

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

All the cases that have not been tested have an undetermined state, so the maintainers can't help whoever opens an issue.

In this case, the maintainer should add a test case for it and add it to the repository etc.. in an endless pain loop.

So, we can discuss about the minimal test suite we need, but yes: 1 test is not enough of a HUGE feature like this.

So I think the minimal test suite for this test suite is:

  • check that the feature works as expected:
    • are decorators encapsulated?
    • is the fastify version check working?
    • are mandatory decorators checks applied?
    • is the required plugins option working?
  • verify how fastify reacts to this configuration (plugin name chaining)

}))

fastify.ready(err => {
t.notOk(err)
dwickern marked this conversation as resolved.
Show resolved Hide resolved
t.ok(fastify.hasDecorator('accessible'))
t.ok(fastify.hasDecorator('alsoAccessible'))
t.notOk(fastify.hasDecorator('encapsulated'))
})
})