Skip to content

perf: use node: prefix to bypass require.cache call for builtins#407

Merged
Uzlopak merged 2 commits intofastify:masterfrom
Fdawgs:perf/builtins
Sep 8, 2023
Merged

perf: use node: prefix to bypass require.cache call for builtins#407
Uzlopak merged 2 commits intofastify:masterfrom
Fdawgs:perf/builtins

Conversation

@Fdawgs
Copy link
Copy Markdown
Member

@Fdawgs Fdawgs commented Sep 7, 2023

@Fdawgs
Copy link
Copy Markdown
Member Author

Fdawgs commented Sep 7, 2023

Seem to be getting test failures locally even on master, using both 18.17.1 and 20.6.0

@gurgunday
Copy link
Copy Markdown
Member

gurgunday commented Sep 7, 2023

It might be because of a test that modifies/mocks a built-in module

@gurgunday
Copy link
Copy Markdown
Member

Modifying static.test.js (L2822) fixes the issue

t.test(
  'register with rootpath that causes statSync to fail with non-ENOENT code',
  (t) => {
    const fastifyStatic = proxyquire('../', {
      'node:fs': { // HERE: fs -> 'node:fs'
        statSync: function statSyncStub (path) {
          throw new Error({ code: 'MOCK' })
        }
      }
    })

    const pluginOptions = {
      root: path.join(__dirname, '/static'),
      wildcard: true
    }
    const fastify = Fastify()
    fastify.register(fastifyStatic, pluginOptions)

    t.teardown(fastify.close.bind(fastify))
    fastify.listen({ port: 0 }, (err) => {
      fastify.server.unref()
      t.ok(err)
      t.end()
    })
  }
)

Copy link
Copy Markdown
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

I was today years old...

Core modules can be identified using the node: prefix, in which case it bypasses the require cache. For instance, require('node:http') will always return the built in HTTP module, even if there is require.cache entry by that name.

Didn't know it

LGTM on green pipeline 👍🏼

@Fdawgs
Copy link
Copy Markdown
Member Author

Fdawgs commented Sep 8, 2023

Happy to mass update rest of the repos with a similar PR at some point this weekend, unless @gurgunday beats me to it 😂

@Uzlopak
Copy link
Copy Markdown
Contributor

Uzlopak commented Sep 8, 2023

What about prefixing npm packages with npm:? Wouldnt we then be deno compatible?

@gurgunday
Copy link
Copy Markdown
Member

gurgunday commented Sep 8, 2023

Happy to mass update rest of the repos with a similar PR at some point this weekend, unless @gurgunday beats me to it 😂

🤣, I just wanted to get the ones I actively use out of the way in case there is a release or something

I would of course prefer if you created a script for it 😁

@Fdawgs
Copy link
Copy Markdown
Member Author

Fdawgs commented Sep 8, 2023

What about prefixing npm packages with npm:? Wouldnt we then be deno compatible?

Can't find anything in node's documentation about supporting this?

@Fdawgs Fdawgs requested a review from Uzlopak September 8, 2023 12:08
Copy link
Copy Markdown
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

@Uzlopak Uzlopak merged commit 4e213cb into fastify:master Sep 8, 2023
@Fdawgs Fdawgs deleted the perf/builtins branch September 8, 2023 12:50
Fdawgs added a commit to fastify/ajv-compiler that referenced this pull request Sep 10, 2023
Fdawgs added a commit to fastify/aws-lambda-fastify that referenced this pull request Sep 10, 2023
Fdawgs added a commit to fastify/compile-schemas-to-typescript that referenced this pull request Sep 10, 2023
Fdawgs added a commit to fastify/create-fastify that referenced this pull request Sep 10, 2023
Fdawgs added a commit to fastify/csrf-protection that referenced this pull request Sep 10, 2023
Fdawgs added a commit to fastify/deepmerge that referenced this pull request Sep 10, 2023
Fdawgs added a commit to fastify/env-schema that referenced this pull request Sep 10, 2023
Fdawgs added a commit to fastify/fast-content-type-parse that referenced this pull request Sep 10, 2023
Fdawgs added a commit to fastify/fastify that referenced this pull request Sep 10, 2023
Fdawgs added a commit to fastify/fastify-autoload that referenced this pull request Sep 10, 2023
Uzlopak pushed a commit to fastify/ajv-compiler that referenced this pull request Sep 10, 2023
Uzlopak pushed a commit to fastify/avvio that referenced this pull request Sep 10, 2023
)

* perf: use `node:` prefix to bypass require.cache call for builtins

See fastify/fastify-static#407

* docs: use `node:` prefix to bypass require.cache call for builtins
Uzlopak pushed a commit to fastify/deepmerge that referenced this pull request Sep 10, 2023
@Fdawgs
Copy link
Copy Markdown
Member Author

Fdawgs commented Sep 15, 2023

@mcollina happy to update Pino repos accordingly also, wdyt?

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 this pull request may close these issues.

4 participants