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

TypeError: Cannot read properties of undefined (reading 'func') #98

Open
nasraldin opened this issue Feb 2, 2024 · 16 comments · May be fixed by #105
Open

TypeError: Cannot read properties of undefined (reading 'func') #98

nasraldin opened this issue Feb 2, 2024 · 16 comments · May be fixed by #105

Comments

@nasraldin
Copy link

nasraldin commented Feb 2, 2024

Hello team,

Please apply the fix to avoid the below issue that throw when using with typescript.

Current ver.

"fastify-overview": "3.7.0",
 "fastify": "4.26.0",

To reproduce the issue:
add the plugin
pnpm add fastify-overview

register to fastify 

import FastifyOverview from 'fastify-overview';
      fastify.register(FastifyOverview, {
        addSource: true, // current this opt is throw error and stoped the app start
        exposeRoute: true,
        exposeRouteOptions: {
          url: '/json-overview',
        },
      });

run your app

result
application stoped to start and throw below error

Failed to start application: TypeError: Cannot read properties of undefined (reading 'func')
    at Boot.manInTheMiddle (/api/node_modules/.pnpm/fastify-overview@3.7.0/node_modules/fastify-overview/index.js:90:107)
    at Boot.markInstance (/api/node_modules/.pnpm/fastify-overview@3.7.0/node_modules/fastify-overview/index.js:30:20)
    at Boot.override (/api/node_modules/.pnpm/fastify@4.26.0/node_modules/fastify/lib/pluginOverride.js:69:56)
    at Plugin.exec (/api/node_modules/.pnpm/avvio@8.2.1/node_modules/avvio/plugin.js:79:33)
    at Boot.loadPlugin (/api/node_modules/.pnpm/avvio@8.2.1/node_modules/avvio/plugin.js:272:10)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

The fix:

I'm just checking in the compiled source code in node modules of your lib to fix but for you you need to fix the root cause in js that fun related to show source

In the manInTheMiddle function, update the check to ensure that _current has a value before assigning it to the source.

old fun is

if (opts.addSource && this) {
    trackStructure.source = this._current.find(loadPipe => loadPipe.func[kSourceRegister] !== undefined).func[kSourceRegister]
}

new code

if (opts.addSource && this) {
  const currentSourcePipe = this._current.find(loadPipe => loadPipe.func[kSourceRegister] !== undefined);
  if (currentSourcePipe) {
    trackStructure.source = currentSourcePipe.func[kSourceRegister];
  }
}

Thank you for creating such an amazing tool!

@matthyk
Copy link
Contributor

matthyk commented Feb 3, 2024

You forgot to await the register of fastifyOverview. See in the docs:

There are 3 things to know:
1. It starts tracking your application after the `await register()` of the plugin:
- what happens before, it is **not** tracked. So **this plugin must be the first one** to be registered.
- it the `register` is not awaited, the structure will **not** be tracked.
2. The application structure can be accessed **after** the Fastify instance is `ready`. If you try to get it before the `ready` status, you will get an error.
3. The structure tracks hooks' name and decorators' name. If you use arrow functions the structure will be useless/unreadable.

@nasraldin
Copy link
Author

Hell @matthyk ,

I have reviewed the documentation and observed your note for await, but it does not seem to be the source of the problem. Please examine the snippet of code below, which utilizes the await.

const defaultOpts = {
      addSource: true, // current this opt is throw error and stoped the app startup
      exposeRoute: true,
      exposeRouteOptions: {
        url: '/json-overview',
      },
};

fastify.log.info('Fastify overview tree structure plugin enabled.');

await fastify.register(FastifyOverview, { ...defaultOpts });

fastify.log.info('The route is exposed at GET /json-overview');

@matthyk
Copy link
Contributor

matthyk commented Feb 3, 2024

I don't get an error with the provided code snippet. Can you provide a minimal reproducible example?

@Eomm
Copy link
Owner

Eomm commented Feb 3, 2024

Please apply the fix to avoid the below issue that throw when using with typescript.

I don't define me as TS dev so I need something where I can just run these commands to replicate the error:

git clone xyz
npm install
npm start

TBH If that item is undefined, it means that the getSource util does not return anything:

pluginFn[kSourceRegister] = getSource()[0]

So, I think the issue relies on how the function returns the source and it is definitely not tested in this setup.

module.exports = function callee () {

@nasraldin
Copy link
Author

nasraldin commented Feb 3, 2024

I have created one test app so you can run and check the issue
jus clone and install node modules then run dev command

https://github.com/nasraldin/test-fastify-overview

Here the line throws the issue

trackStructure.source = this._current.find(loadPipe => loadPipe.func[kSourceRegister] !== undefined).func[kSourceRegister]

@matthyk
Copy link
Contributor

matthyk commented Feb 14, 2024

@nasraldin @Eomm

I now had the time to take a closer look at the problem and I have found the cause. The cause of this error is the wrapRegister method when using ECMAScript modules. See the code snippet below.

fastify-overview/index.js

Lines 120 to 126 in c1785f8

instance.register = function wrapRegister (pluginFn, opts) {
if (pluginOpts.addSource) {
// this Symbol is processed by the `onRegister` hook if necessary
pluginFn[kSourceRegister] = getSource()[0]
}
return originalRegister.call(this, pluginFn, opts)
}

The wrapRegister function expects a plugin function (the pluginFn parameter) but depending on how the register method was called, the pluginFn has a different value. Below some example register calls and the corresponding value of the pluginFn parameter.

import Fastify from 'fastify'
import fastifyOverview from 'fastify-overview'
import plugin from './plugin.js'

const app = Fastify()
await app.register(fastifyOverview)

app.register(plugin)

--> pluginFn would be the (async) function imported from plugin.js

import Fastify from 'fastify'
import fastifyOverview from 'fastify-overview'

const app = Fastify()
await app.register(fastifyOverview)

app.register(import('./plugins.js'))

--> pluginFn would be a Promise<{ default: pluginFn }>

import Fastify from 'fastify'
import fastifyOverview from 'fastify-overview'

const app = Fastify()
await app.register(fastifyOverview)

app.register(await import('./plugins.js'))

--> pluginFn would be the same as the second but without the Promise so, { default: pluginFn }

So the problem is that the plugin function can also be a Promise or a Module Namespace Object. The Second point could be easily fixed with a simple check, like Avvio does with this function. If the plugin is a Promise we would have to await it before we continue, but I can't tell if we can just make the method async.

See here for minimal reproducible example.

@Eomm
Copy link
Owner

Eomm commented Feb 14, 2024

It seems we have an actionable! Great analysis!

f the plugin is a Promise we would have to await it before we continue, but I can't tell if we can just make the method async.

This is dangerous because we are no more watching the actual runtime status, but we are doing business logic. In this case I would just try to get all the info we can and check where we could store them since the pluginFn seems not an option

@matthyk
Copy link
Contributor

matthyk commented Feb 21, 2024

@Eomm

While I was analysing the issue, I found another problem. If you run npm run load in my example project you get the following overview output.

{
  "id": 0.6509491979399917,
  "name": "fastify -> fastify-overview",
  "children": [
    {
      "id": 0.2302123930057709,
      "name": "default",
      "children": [],
      "routes": [],
      "decorators": {
        "decorate": [
          {
            "name": "num",
            "type": "number",
            "source": {
              "stackIndex": 0,
              "fileName": "file:///Users/.../fastify-overview-issue-98/plugin.js",
              "relativeFileName": "file:///plugin.js",
              "lineNumber": 2,
              "columnNumber": 9,
              "functionName": "default",
              "typeName": null,
              "methodName": null
            }
          }
        ],
        "decorateRequest": [],
        "decorateReply": []
      },
      "hooks": {
        "onRequest": [],
        "preParsing": [],
        "preValidation": [],
        "preHandler": [],
        "preSerialization": [],
        "onError": [],
        "onSend": [],
        "onResponse": [],
        "onTimeout": [],
        "onRequestAbort": [],
        "onListen": [],
        "onReady": [],
        "preClose": [],
        "onClose": [],
        "onRoute": [],
        "onRegister": []
      },
      "source": {
        "stackIndex": 0,
        "fileName": "node:internal/process/task_queues",
        "relativeFileName": "node:internal/process/task_queues",
        "lineNumber": 82,
        "columnNumber": 21,
        "functionName": "processTicksAndRejections",
        "typeName": "process",
        "methodName": null
      }
    }
  ],
  "routes": [],
  "decorators": {
    "decorate": [],
    "decorateRequest": [],
    "decorateReply": []
  },
  "hooks": {
    "onRequest": [],
    "preParsing": [],
    "preValidation": [],
    "preHandler": [],
    "preSerialization": [],
    "onError": [],
    "onSend": [],
    "onResponse": [],
    "onTimeout": [],
    "onRequestAbort": [],
    "onListen": [],
    "onReady": [],
    "preClose": [],
    "onClose": [],
    "onRoute": [],
    "onRegister": []
  }
}

While the source object of the num decorator is correct the source object of the plugin itself is not correct, since is contains node:internal/process/task_queues as fileName.

@Eomm
Copy link
Owner

Eomm commented Feb 21, 2024

Maybe we need to improve this filter:

function noNodeModules (call) {
const file = call.getFileName()
return file && file.indexOf('node_modules') === -1
}

@matthyk
Copy link
Contributor

matthyk commented Feb 21, 2024

If stack.map(c => c.getFileName()) is run on the array on which this filter method is then executed, you get this output.

[
  "/Users/.../fastify-overview-issue-98/node_modules/fastify-overview/index.js",
  "/Users/.../fastify-overview-issue-98/node_modules/fastify/lib/pluginOverride.js",
  "/Users/.../fastify-overview-issue-98/node_modules/avvio/boot.js",
  "node:internal/process/task_queues"
]

The array therefore does not contain the correct value at all.

@matthyk
Copy link
Contributor

matthyk commented Feb 21, 2024

I can no longer reproduce this output. it seems that I had some strange side effects 😄

@matthyk matthyk linked a pull request Feb 29, 2024 that will close this issue
@matthyk
Copy link
Contributor

matthyk commented Feb 29, 2024

@Eomm I have opened #105 to discuss a possible solution. But if you look at the failed tests, you can also see another problem. As soon as you use ECMAScript modules, you no longer get the correct fileName in the source. See https://github.com/Eomm/fastify-overview/actions/runs/8103392570/job/22147905402?pr=105#step:5:74

@Eomm
Copy link
Owner

Eomm commented Mar 1, 2024

Thanks for taking care of this issue.

TBH I would check how APMs do it

@matthyk
Copy link
Contributor

matthyk commented Mar 10, 2024

I've been looking for a solution for some time now, but haven't found one yet. But maybe we should solve the problem with the sources separately from the issues with the registration of ECMAScript modules, since the two things have nothing to do with each other.

@Eomm
Copy link
Owner

Eomm commented Mar 25, 2024

I did not understand: are you suggesting to add a condition to avoid the crash?

@matthyk
Copy link
Contributor

matthyk commented Apr 22, 2024

Current situation: The plugin crashes if a Module Namespace Object or a Promise is registered

My suggestion:

  • For Module Namespace Object: Check if the current plugin is a Module Namespace Object, and if so, register the corresponding function correctly
  • For Promises: Let the plugin crash, since we currently do not have a solution but we could print a warning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants