fastify / fastify-type-provider-json-schema-to-ts

A Type Provider for json-schema-to-ts
MIT License
36 stars 9 forks source link

Documentation nitpick: possibly add minimum node engine version #34

Closed niktekusho closed 1 year ago

niktekusho commented 1 year ago

Prerequisites

Issue

Very nice library!

With this issue, I only want to raise a minor awareness issue (which could be addressed by a small doc note or other possibly more "invasive" solutions).

I (and other people from what I've witnessed on Reddit and StackOverflow) found it difficult to understand why the library wasn't working like it was supposed to.

The main problem comes from something other than the code we write but from one's dev environment.

TLDR: a peer dependency requires Node 16, but since it's not enforced by npm (at least not by default), you can run into "silent breakages" when you use an older version of Node (example 14).

You can also experience this issue when cloning this repository and trying to npm install && npm test with Node < 16.

Reproduction on a small example

Setup environment first (nvm required)

nvm install 14
nvm  use 14
node -v
# should output "v14.xx.xx"

Basic example:

import fastify from 'fastify'

import { FastifyPluginAsyncJsonSchemaToTs } from "@fastify/type-provider-json-schema-to-ts"

const server = fastify()

const pingPlugin: FastifyPluginAsyncJsonSchemaToTs = async function (fastify, opts) {

    fastify.get('/ping/:times?', {
        schema: {
            params: {
                type: 'object',
                properties: {
                    times: { 
                        default: 1,
                        type: 'number'
                    }
                }
            } as const
        }
    }, async (request) => {
        const {times = 1} = request.params

        if (times > 1 && times < 10) {
            const oCount = 'o'.repeat(times)
            return `p${oCount}ng`
        }

        return 'pong!'
    })
}

server.register(pingPlugin)

server.listen({ port: 8080 }, (err, address) => {
    if (err) {
        console.error(err)
        process.exit(1)
    }
    console.log(`Server listening at ${address}`)
})

Project setup:

npm init -y
npm i fastify
npm i -D @fastify/type-provider-json-schema-to-ts @types/node typescript

If you use an older version of Node (example 14 LTS), you might notice the following warnings raised by npm:

npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'json-schema-to-ts@2.6.2',
npm WARN EBADENGINE   required: { node: '>=16' },
npm WARN EBADENGINE   current: { node: 'v14.21.2', npm: '9.5.0' }
npm WARN EBADENGINE }

And this is where the issue lies: these warnings effectively hinder this lib's ability to function properly since the required dependency is not installed. And since they are warnings, people might not notice them (or, at least, that's what happened to me).

You might ask: what solved it? Just run nvm use 18 or nvm use 16 (if you use nvm) and reinstall dependencies.

Just FYI:

What I propose to acknowledge this issue

  1. Add a small note in the README that mentions this possible breakage on older Node.js version (my preferred approach to at least acknowledge this issue)
  2. Do nothing since Node.js 14 LTS is due by 2023-04-30
  3. "Fix" the semver version declared in package.json for json-schema-to-ts (how? depends on this lib's support policy over Node core versions)
  4. Add the required node engine version to this lib, too (maybe it will be enforced by npm if dependencies are "direct" vs. "indirect", needs testing)
climba03003 commented 1 year ago

Explicitly documentation requires continuous updates with the information and there are no way for us to know the changes unless someone open an issue. The main reason is most of the folks do not use this plugin (or even TypeScript).

I am ok with tell the user to check the supported node version in json-schema-to-ts. But not explicitly tell we are not supporting node@14.

Given that the CI are green in both node@12 and node@14, I believe it is safe to use inside those version for now.

niktekusho commented 1 year ago

Explicitly documentation requires continuous updates with the information and there are no way for us to know the changes unless someone open an issue. The main reason is most of the folks do not use this plugin (or even TypeScript).

I am ok with tell the user to check the supported node version in json-schema-to-ts. But not explicitly tell we are not supporting node@14.

Given that the CI are green in both node@12 and node@14, I believe it is safe to use inside those version for now.

After your input, I checked again on a freshly created project and I can't seem to find a way to reproduce the issue with node@14. I'll close this, hoping it will bring some ideas to others who encounter similar issues.