feat: core functionality added #1

Ceres6 · 2022-11-16T13:09:31Z

Core functionality added with an onRoute hook to add constraints to all routes declared after registration on the same or descendent scope

resolves fastify/fastify#3615

.nyc_output/039a84fc-6305-4919-b79b-b9f1c64c1724.json

simoneb

Overall very good in the sense that the plugin functionality seems correct and complete 🚀

I've added a few comments around mostly minor things and style

simoneb · 2022-11-16T17:22:21Z

.husky/pre-commit

@@ -2,3 +2,4 @@
 . "$(dirname -- "$0")/_/husky.sh"

 npx lint-staged
+npm test


I prefer not to run tests on pre-commit, it slows down the development cycle

README.md

simoneb · 2022-11-17T08:37:11Z

README.md

+
+## Usage
+
+Register fastifyConstraints as a Fastify plugin.


let's name the plugin consistently, it's called fastify-constraints. Also, consider wrapping it in backticks to format it as code

simoneb · 2022-11-17T08:38:12Z

index.d.ts

+
+export { fastifyConstraints, FastifyConstraintsOptions }
+
+export default fastifyConstraints


missing new line at end of file

simoneb · 2022-11-17T08:38:29Z

index.d.ts

@@ -0,0 +1,9 @@
+type FastifyConstraintsOptions = {
+  constraints: { [name: string]: any}


formatting (missing space before closing brace)

I'll be removing the file as the new implementation won't take any options

simoneb · 2022-11-17T08:40:02Z

index.js

+  fastify.addHook('onRoute', function hook(routeOptions) {
+    routeOptions.constraints = {
+      ...options.constraints,
+      ...(routeOptions?.constraints || {})


|| {} is unnecessary. destructuring a falsy value is a noop

simoneb · 2022-11-17T08:40:36Z

test/index.test.js

+import fastifyConstraints from '../index.js'
+
+test('it should add constraints to all routes', async t => {
+  t.plan(2)


this is unnecessary, remove

simoneb · 2022-11-17T08:41:07Z

test/index.test.js

+  fastify.get('/', () => {})
+  fastify.post('/some-route', () => {})
+
+  t.equal(


nit: instead of comparing with true, you can simply use t.ok

simoneb · 2022-11-17T08:42:03Z

test/index.test.js

+  await fastify.register((instance, opts, done) => {
+    fastify.get('/', () => {})
+
+    done()


I guess there's no need for the callback based registration syntax. Use an async function instead

climba03003

It would be better to allow

fastify.register(function(fastify, option) {
  fastify.get('/', () => {})
}, { constraints: { foo: 'bar' } })

The plugin would not take any option and create a property to store the constraints.

const symbol = Symbol.for('kConstraints')
fastify.decorate(symbol, null)

fastify.addHook('onRegister', function(instance, options) {
  if(options.constraints) {
    instance[symbol] = { ...instance[symbol], ...options.constraints }
  }
})

fastify.addHook('onRoute', function(routeOptions) {
  if(this[symbol]) {
    routeOptions.constraints = { ...this[symbol], ...routeOptions.constraints }
  }
})

Ceres6 · 2022-11-17T09:56:51Z

It would be better to allow

fastify.register(function(fastify, option) {
  fastify.get('/', () => {})
}, { constraints: { foo: 'bar' } })

The plugin would not take any option and create a property to store the constraints.

const symbol = Symbol.for('kConstraints')
fastify.decorate(symbol, null)

fastify.addHook('onRegister', function(instance, options) {
  if(options.constraints) {
    instance[symbol] = { ...instance[symbol], ...options.constraints }
  }
})

fastify.addHook('onRoute', function(routeOptions) {
  if(this[symbol]) {
    routeOptions.constraints = { ...this[symbol], ...routeOptions.constraints }
  }
})

Hi @climba03003 Thanks for pointing it out. I was already working on making it that way. It should work that way now. Let me know if you have any other comments

index.js

climba03003 · 2022-11-17T10:12:13Z

index.js

+      return
+    }
+
+    instance.addHook('onRoute', function hook(routeOptions) {


Not sure if it is good or not. It may add a lot of onRoute function.

I see your point, we would have as many onRoute hooks as plugins with constraints. I think your way would be more efficient. Let me change that.

Co-authored-by: KaKa <climba03003@gmail.com>

simoneb

LGTM 🚀

simoneb · 2022-11-18T10:02:33Z

examples/imported-routes.cjs

@@ -0,0 +1,15 @@
+const Fastify = require('fastify')


let's remove the cjs ones

Ceres6 added 4 commits November 16, 2022 14:05

feat: core functionality added

6822f79

feat: plugin modified to merge route level constraints

ac68d17

chore: changed syntax for test coverage

0c3e369

chore: tests added to commit hook

acb9e95

Ceres6 marked this pull request as ready for review November 16, 2022 15:28

Ceres6 self-assigned this Nov 16, 2022

simoneb reviewed Nov 16, 2022

View reviewed changes

.nyc_output/039a84fc-6305-4919-b79b-b9f1c64c1724.json Outdated Show resolved Hide resolved

chore: .gitignore updated to ignore test files

8b5815b

Ceres6 marked this pull request as draft November 17, 2022 08:05

simoneb reviewed Nov 17, 2022

View reviewed changes

simoneb mentioned this pull request Nov 17, 2022

Support constraints when registering plugins fastify/fastify#3615

Closed

2 tasks

climba03003 reviewed Nov 17, 2022

View reviewed changes

feat: plugin modified to allow constraints on plugin registration

93eaff1

Ceres6 marked this pull request as ready for review November 17, 2022 09:57

climba03003 reviewed Nov 17, 2022

View reviewed changes

Ceres6 and others added 3 commits November 17, 2022 11:51

Update index.js

a9400af

Co-authored-by: KaKa <climba03003@gmail.com>

feat: plugin modified to create only one onRoute hook

2181d72

chore: added test on scope inheritance

c954b74

simoneb approved these changes Nov 18, 2022

View reviewed changes

examples/imported-routes.cjs Outdated

@@ -0,0 +1,15 @@

const Fastify = require('fastify')

Copy link

Member

simoneb Nov 18, 2022

Choose a reason for hiding this comment

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

let's remove the cjs ones

Ceres6 added 2 commits November 18, 2022 11:08

chore: remove cjs examples

fd3f114

chore: change version to 0.0.1

1653735

simoneb merged commit 357a2e3 into master Nov 18, 2022

optic-release-automation bot mentioned this pull request Nov 18, 2022

[OPTIC-RELEASE-AUTOMATION] release/v1.0.0 #2

Merged

simoneb deleted the feat/plugin branch November 18, 2022 10:23

optic-release-automation bot mentioned this pull request Dec 15, 2022

[OPTIC-RELEASE-AUTOMATION] release/v1.0.1 #19

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: core functionality added #1

feat: core functionality added #1

Ceres6 commented Nov 16, 2022 •

edited

simoneb left a comment

simoneb Nov 16, 2022

simoneb Nov 17, 2022

simoneb Nov 17, 2022

simoneb Nov 17, 2022

Ceres6 Nov 17, 2022

simoneb Nov 17, 2022

simoneb Nov 17, 2022

simoneb Nov 17, 2022

simoneb Nov 17, 2022

climba03003 left a comment •

edited

Ceres6 commented Nov 17, 2022 •

edited

climba03003 Nov 17, 2022

Ceres6 Nov 17, 2022 •

edited

simoneb left a comment

simoneb Nov 18, 2022


		export { fastifyConstraints, FastifyConstraintsOptions }

		export default fastifyConstraints

		@@ -0,0 +1,9 @@
		type FastifyConstraintsOptions = {
		constraints: { [name: string]: any}

feat: core functionality added #1

feat: core functionality added #1

Conversation

Ceres6 commented Nov 16, 2022 • edited

simoneb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

climba03003 left a comment • edited

Choose a reason for hiding this comment

Ceres6 commented Nov 17, 2022 • edited

Choose a reason for hiding this comment

Ceres6 Nov 17, 2022 • edited

Choose a reason for hiding this comment

simoneb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ceres6 commented Nov 16, 2022 •

edited

climba03003 left a comment •

edited

Ceres6 commented Nov 17, 2022 •

edited

Ceres6 Nov 17, 2022 •

edited