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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove app.use() #3502

Closed
2 tasks done
mcollina opened this issue Nov 29, 2021 · 20 comments
Closed
2 tasks done

Remove app.use() #3502

mcollina opened this issue Nov 29, 2021 · 20 comments
Labels
feature request New feature to be added good first issue Good for newcomers v4.x Issue or pr related to Fastify v4
Milestone

Comments

@mcollina
Copy link
Member

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

馃殌 Feature Proposal

I think we should be removing app.use() in the next release, i.e.

fastify/fastify.js

Lines 327 to 330 in a174829

// We are adding `use` to the fastify prototype so the user
// can still access it (and get the expected error), but `decorate`
// will not detect it, and allow the user to override it.
Object.setPrototypeOf(fastify, { use })
.

Motivation

We should not be encouraging people to use middlewares. Both middie and fastify-express will still be there and maintained.

Example

No response

@mcollina mcollina added feature request New feature to be added v4.x Issue or pr related to Fastify v4 labels Nov 29, 2021
@mcollina mcollina added this to the v4.0.0 milestone Nov 29, 2021
@mcollina mcollina added the good first issue Good for newcomers label Nov 29, 2021
@jsumners
Copy link
Member

Please do this. I wouldn't be upset if we also deprecate middie and fastify-express with v4.

@mcollina
Copy link
Member Author

I think they are useful plugins, we should keep having them but reduce their visibility.

@genzyy
Copy link
Contributor

genzyy commented Nov 29, 2021

Can I take this? How should I go about this like removing use will do it I guess but deleting entire line will be better or am I missing something here?

@jsumners
Copy link
Member

First thing we should probably do is issue a new v3 minor that adds a depreciation warning.

@genzyy
Copy link
Contributor

genzyy commented Nov 29, 2021

First thing we should probably do is issue a new v3 minor that adds a depreciation warning.

For this should the docs be updated here?

@jsumners
Copy link
Member

That would be part of it. The main thing would be adding a new deprecation error and triggering it when the method is invoked.

@mcollina
Copy link
Member Author

it's already throwing an error, we can just remove it.

@genzyy
Copy link
Contributor

genzyy commented Nov 30, 2021

it's already throwing an error, we can just remove it.

Should I make this change on a new branch while keeping next as a base and make a pull request to next branch?

@climba03003
Copy link
Member

it's already throwing an error, we can just remove it.

Should I make this change on a new branch while keeping next as a base and make a pull request to next branch?

Yes

@genzyy
Copy link
Contributor

genzyy commented Nov 30, 2021

Hey, so to fix this I made a new branch while keeping next as the base. But when I run npm run test, it gives me some errors. How should I commit the changes?

@climba03003
Copy link
Member

You need to remove the corresponding test for .use.

test('Should throw if the basic use API has not been overridden', t => {
t.plan(1)
const fastify = Fastify()
try {
fastify.use()
t.fail('Should throw')
} catch (err) {
t.ok(err instanceof FST_ERR_MISSING_MIDDLEWARE)
}
})

fastify.register((instance, opts, done) => {
try {
instance.use()
t.fail('Should throw')
} catch (err) {
t.ok(err instanceof FST_ERR_MISSING_MIDDLEWARE)
}
done()
})

@genzyy
Copy link
Contributor

genzyy commented Nov 30, 2021

You need to remove the corresponding test for .use.

test('Should throw if the basic use API has not been overridden', t => {
t.plan(1)
const fastify = Fastify()
try {
fastify.use()
t.fail('Should throw')
} catch (err) {
t.ok(err instanceof FST_ERR_MISSING_MIDDLEWARE)
}
})

fastify.register((instance, opts, done) => {
try {
instance.use()
t.fail('Should throw')
} catch (err) {
t.ok(err instanceof FST_ERR_MISSING_MIDDLEWARE)
}
done()
})

Removed these tests but I still have errors from multiple test files
image

@mcollina
Copy link
Member Author

How should I commit the changes?

git commit -n to skip the pre-commit hooks.


Looking at your errors, I would recommend to rm -rf node_modules; npm i as it seems you are missing some deps (joi).

As for the rest, you'd need to look why they are failining.

@genzyy
Copy link
Contributor

genzyy commented Nov 30, 2021

How should I commit the changes?

git commit -n to skip the pre-commit hooks.

Looking at your errors, I would recommend to rm -rf node_modules; npm i as it seems you are missing some deps (joi).

As for the rest, you'd need to look why they are failining.

Okay, I got here somehow
image

What about this one?
I removed the two test function mentioned above but I guess more needs to be removed.

@climba03003
Copy link
Member

Update the t.plan to match the actual test number.

@genzyy
Copy link
Contributor

genzyy commented Nov 30, 2021

Update the t.plan to match the actual test number.

Fixed all of the above
But I got these errors popped up

> fastify@4.0.0-dev test:typescript
> tsc test/types/import.ts && tsd

types/logger.d.ts:18:33 - error TS2694: Namespace 'pino' has no exported member 'SerializerFn'.

18 export type SerializerFn = pino.SerializerFn
                                   ~~~~~~~~~~~~

types/logger.d.ts:23:38 - error TS2694: Namespace 'pino' has no exported member 'BaseLogger'.

23 export type FastifyBaseLogger = pino.BaseLogger & {
                                        ~~~~~~~~~~

types/logger.d.ts:27:34 - error TS2694: Namespace 'pino' has no exported member 'PrettyOptions'.

27 export type PrettyOptions = pino.PrettyOptions & { suppressFlushSyncWarning?: boolean }
                                    ~~~~~~~~~~~~~


Found 3 errors.

@climba03003
Copy link
Member

Update the t.plan to match the actual test number.

Fixed all of the above But I got these errors popped up

> fastify@4.0.0-dev test:typescript
> tsc test/types/import.ts && tsd

types/logger.d.ts:18:33 - error TS2694: Namespace 'pino' has no exported member 'SerializerFn'.

18 export type SerializerFn = pino.SerializerFn
                                   ~~~~~~~~~~~~

types/logger.d.ts:23:38 - error TS2694: Namespace 'pino' has no exported member 'BaseLogger'.

23 export type FastifyBaseLogger = pino.BaseLogger & {
                                        ~~~~~~~~~~

types/logger.d.ts:27:34 - error TS2694: Namespace 'pino' has no exported member 'PrettyOptions'.

27 export type PrettyOptions = pino.PrettyOptions & { suppressFlushSyncWarning?: boolean }
                                    ~~~~~~~~~~~~~


Found 3 errors.

It is expected to be fail as pinojs/pino#1195 introduce the type breaking in certain area.
You can submit the PR, it should be fixed in another PR.

@genzyy
Copy link
Contributor

genzyy commented Nov 30, 2021

It is expected to be fail as pinojs/pino#1195 introduce the type breaking in certain area. You can submit the PR, it should be fixed in another PR.

Got it!
Also we need to add a note in the current version stating the deprecation of app.use() in the next release right?

@jsumners
Copy link
Member

I think they are useful plugins, we should keep having them but reduce their visibility.

Issues like fastify/fastify-express#61 are why I think we should deprecate the middie and fastify-express modules. People are not using them to "migrate" to Fastify. They're keeping their old Express stuff around indefinitely because we continue to support it. This is going to continue to cause problems and maintenance burden as these users continue to try and add more and more new technologies on top.

In short: I think we have given enough lifecycles to facilitate migrating. It's time to move forward.

@Eomm
Copy link
Member

Eomm commented Dec 14, 2021

Done in #3506

@Eomm Eomm closed this as completed Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature to be added good first issue Good for newcomers v4.x Issue or pr related to Fastify v4
Projects
None yet
Development

No branches or pull requests

5 participants