feat: add routeSchema and routeConfig + switching context handling #4216

metcoder95 · 2022-08-23T21:47:40Z

Implements #4149

Description:

As stated in #4149, this PR introduces:

Hide the Context object tight to the route through a Symbol
Expose a public API to fetch pieces of Route config through the request object with the signature of request.routeContext.

Notes

This is an initial implementation and can be changed over time through different iterations until we're happy with the shape. This is the reason why is pointing to the next major version of Fastify
The current settings exposed right now are:
- schema
- config
- logLevel (which is also configurable)
Testing is pending, will be added later within this PR

Open Questions

Should we make the exposed properties "read-only"?
What else should be exposed through the public API?

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

Signed-off-by: Matteo Collina <hello@matteocollina.com>

Eomm

Should we make the exposed properties "read-only"?

Yes: any changes will not be processed by fastify.

What else should be exposed through the public API?

I think we should start small instead of adding unwanted stuff.
The initial request was to access the schema only

lib/reply.js

Eomm · 2022-09-05T16:58:54Z

lib/context.js

+        return context.config
+      }
+    },
+    logLevel: {


Why this addition?

I was playing a bit and might be beneficial to have some sort of single place where to look for specific settings of the route definition. I was thinking of a single read-only approach but wanted to get your thoughts on this, as might/might-not have sense. wdyt?

I would not say it is non-sense, it is just something nobody asked for so I would work on it in a separate PR if necessary instead of adding too much code on this one

Has sense, let's keep it clean 👍
Done in 2c18837

lib/request.js

mcollina · 2022-09-13T14:05:31Z

Is this regressing on perf?

Eomm · 2022-09-13T17:56:36Z

One failing test

I think we could target main instead of next.
Is @fastify/core against it?

climba03003 · 2022-09-13T18:20:04Z

One failing test

The failing test is non-related to the PR changes.
It is related to #4260

It just need to rebase.

metcoder95 · 2022-09-13T20:09:11Z

Is this regressing on perf?

@mcollina
Thankfully no so far, these are the results:

feat/4149 -> next

main

Specs

    Hardware Overview:

      Model Name: MacBook Pro
      Model Identifier: MacBookPro18,1
      Chip: Apple M1 Pro
      Total Number of Cores: 10 (8 performance and 2 efficiency)
      Memory: 16 GB
      System Firmware Version: 7459.141.1
      OS Loader Version: 7459.141.1

There is even an extremely small improvement within this branch. But no regression so far. Maybe someone else can double-check this?

metcoder95 · 2022-09-13T20:10:08Z

The failing test is non-related to the PR changes.
It is related to #4260

It just need to rebase.

Can I have some help rebasing on next so I can do the proper? 🙂

# Conflicts: # fastify.js # package.json

climba03003 · 2022-09-14T05:28:21Z

The failing test is non-related to the PR changes.
It is related to #4260
It just need to rebase.

Can I have some help rebasing on next so I can do the proper? 🙂

I have merged the main branch into next.
Can you check if rebase works in your local branch?
If it does works, I believe it can be switched from next to main safely.

metcoder95 · 2022-09-14T06:53:08Z

Done @climba03003, thanks for the rebase on next 🙂

docs/Reference/Request.md

fastify.js

Eomm · 2022-09-14T16:10:33Z

test/router-options.test.js

@@ -130,7 +87,8 @@ test('Should honor maxParamLength option', t => {
  })
 })

-test('Should expose router options via getters on request and reply', t => {
+// TODO: fix once agreed on the new router options object
+test('Should expose router options via getters on request and reply', { todo: true }, t => {


package.json

docs/Reference/Reply.md

zrosenbauer

Only question I have is there a number of "TODOS" added and usually I link those to issues if I can, especially things like "remove in v5".

(I'm also new to community here so wondering if it handled differently)

metcoder95 · 2022-09-20T07:28:10Z

Only question I have is there a number of "TODOS" added and usually I link those to issues if I can, especially things like "remove in v5".

(I'm also new to community here so wondering if it handled differently)

Hey! 🙂
Not so sure how to handle those as the release of v5 will happen over 2023. No so sure if we want to keep the issue hanging all that time. We can track it in some other way, but now that you noticed the TODO I actually need to remove them as it's not needed anymore

metcoder95 · 2022-09-21T06:43:53Z

Change has been done and pushed, @Eomm can you have a second look? 🙂

zrosenbauer · 2022-09-22T18:57:56Z

Only question I have is there a number of "TODOS" added and usually I link those to issues if I can, especially things like "remove in v5".
(I'm also new to community here so wondering if it handled differently)

Hey! 🙂 Not so sure how to handle those as the release of v5 will happen over 2023. No so sure if we want to keep the issue hanging all that time. We can track it in some other way, but now that you noticed the TODO I actually need to remove them as it's not needed anymore

SWEET!

I personally don't love TODOS in code getting merged (w/o an issue reference) as they fall into the nether and are (sometimes) important. Don't know if core maintainers have opinions... and don't want to create a problem where there might not be one 😄

zrosenbauer

LGTM

Eomm

Last chores and then LGTM 👍🏽

Eomm · 2022-09-24T10:34:48Z

docs/Reference/Reply.md

@@ -84,7 +84,6 @@ object that exposes the following functions and properties:
  from Node core.
 - `.log` - The logger instance of the incoming request.
 - `.request` - The incoming request.
- `.context` - Access the [Request's context](./Request.md) property.


we should kepp it right now

Also done in f3be7b5

docs/Reference/Request.md

Eomm · 2022-09-24T10:40:15Z

test/router-options.test.js

-    t.equal(reply.context.config.url, '/test/:id')
-    t.equal(reply.context.config.method, 'GET')


I would keep these two lines: they will fail when we will remove the context option but till then we need to test it

Sure, done in f3be7b5

mcollina · 2022-09-26T08:09:53Z

Can you please include a benchmark?

github-actions · 2022-09-26T14:36:05Z

Node: 14
PR: [1] 1706k requests in 30.05s, 320 MB read
MAIN: [1] 1699k requests in 30.06s, 319 MB read

Node: 16
PR: [1] 914k requests in 30.09s, 172 MB read
MAIN: [1] 932k requests in 30.13s, 175 MB read

Node: 18
PR: [1] 850k requests in 30.06s, 160 MB read
MAIN: [1] 836k requests in 30.07s, 157 MB read

mcollina

lgtm

docs/Reference/Request.md

github-actions · 2023-09-28T00:19:58Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

bumped v5.0.0-dev

2c27e70

Signed-off-by: Matteo Collina <hello@matteocollina.com>

metcoder95 changed the base branch from main to next August 23, 2022 21:48

metcoder95 force-pushed the feat/4149 branch from b95ac6c to 6aa3fff Compare September 5, 2022 16:19

metcoder95 marked this pull request as ready for review September 5, 2022 16:21

Eomm reviewed Sep 5, 2022

View reviewed changes

Merge branch 'main' into next

db75a3a

# Conflicts: # fastify.js # package.json

metcoder95 added 11 commits September 14, 2022 08:35

feat: initial implementation of context handling switch

e424f2c

refactor: hide context for reply/request

ccc8535

fix: remove trailling space

c6b3e60

tests: adjust tests based on new way of handling context

e95e8e8

refactor: rename route context symbol

2c45ed9

feat: expose route context through request object

3c1e288

chore: add temporary context

968fca0

refactor: reverting log-level

7cef061

refactor: export config/schema into its own naming

f1c2c57

docs: update docs reflecting changes to request/reply

a16714a

tests: extend coverage

aa5ed77

metcoder95 force-pushed the feat/4149 branch from f22015c to aa5ed77 Compare September 14, 2022 06:35

Eomm changed the base branch from next to main September 14, 2022 16:08

Eomm reviewed Sep 14, 2022

View reviewed changes

metcoder95 added 2 commits September 15, 2022 09:06

refactor: initial cleanup

489e79b

tests: remove todo and adjust config test

0da3aa5

metcoder95 requested a review from Eomm September 16, 2022 07:34

zrosenbauer reviewed Sep 19, 2022

View reviewed changes

chore: emit warning on access to Request#context

21e825a

zrosenbauer approved these changes Sep 22, 2022

View reviewed changes

Eomm reviewed Sep 24, 2022

View reviewed changes

Eomm linked an issue Sep 24, 2022 that may be closed by this pull request

Expose schema in request object #4149

Closed

2 tasks

Eomm changed the title ~~feat!: Switch context handling~~ feat: add routeSchema and routeConfig + switching context handling Sep 24, 2022

Eomm added the semver-minor Issue or PR that should land as semver minor label Sep 25, 2022

RafaelGSS added the benchmark Label to run benchmark against PR and main branch label Sep 26, 2022

github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Sep 26, 2022

mcollina approved these changes Sep 26, 2022

View reviewed changes

refactor: apply review

f3be7b5

Eomm reviewed Sep 27, 2022

View reviewed changes

docs/Reference/Request.md Show resolved Hide resolved

docs: restore context docs

df5642d

Eomm approved these changes Sep 27, 2022

View reviewed changes

Eomm merged commit 7ffefaf into fastify:main Sep 27, 2022

Eomm deleted the feat/4149 branch September 27, 2022 22:04

SimenB mentioned this pull request Nov 25, 2022

Access handler name #4439

Closed

2 tasks

github-actions bot locked as resolved and limited conversation to collaborators Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add routeSchema and routeConfig + switching context handling #4216

feat: add routeSchema and routeConfig + switching context handling #4216

metcoder95 commented Aug 23, 2022 •

edited

Eomm left a comment

Eomm Sep 5, 2022

metcoder95 Sep 5, 2022

Eomm Sep 11, 2022

metcoder95 Sep 13, 2022

mcollina commented Sep 13, 2022

Eomm commented Sep 13, 2022

climba03003 commented Sep 13, 2022

metcoder95 commented Sep 13, 2022

metcoder95 commented Sep 13, 2022

climba03003 commented Sep 14, 2022

metcoder95 commented Sep 14, 2022

Eomm Sep 14, 2022

zrosenbauer left a comment

metcoder95 commented Sep 20, 2022

metcoder95 commented Sep 21, 2022

zrosenbauer commented Sep 22, 2022

zrosenbauer left a comment

Eomm left a comment

Eomm Sep 24, 2022

metcoder95 Sep 27, 2022

Eomm Sep 24, 2022

metcoder95 Sep 27, 2022

mcollina commented Sep 26, 2022

github-actions bot commented Sep 26, 2022

mcollina left a comment

github-actions bot commented Sep 28, 2023

		t.equal(reply.context.config.url, '/test/:id')
		t.equal(reply.context.config.method, 'GET')

feat: add routeSchema and routeConfig + switching context handling #4216

feat: add routeSchema and routeConfig + switching context handling #4216

Conversation

metcoder95 commented Aug 23, 2022 • edited

Checklist

Eomm 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

mcollina commented Sep 13, 2022

Eomm commented Sep 13, 2022

climba03003 commented Sep 13, 2022

metcoder95 commented Sep 13, 2022

metcoder95 commented Sep 13, 2022

climba03003 commented Sep 14, 2022

metcoder95 commented Sep 14, 2022

Choose a reason for hiding this comment

zrosenbauer left a comment

Choose a reason for hiding this comment

metcoder95 commented Sep 20, 2022

metcoder95 commented Sep 21, 2022

zrosenbauer commented Sep 22, 2022

zrosenbauer left a comment

Choose a reason for hiding this comment

Eomm 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

mcollina commented Sep 26, 2022

github-actions bot commented Sep 26, 2022

mcollina left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 28, 2023

metcoder95 commented Aug 23, 2022 •

edited