make type definitons `"module": "nodenext"` compatible by wight554 · Pull Request #184 · fastify/fastify-cookie

wight554 · 2022-05-25T18:05:35Z

see Losing types when importing cjs module using default export with nodenext modules microsoft/TypeScript#48845

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

climba03003

Not sure which TypeScript update make it not work.
But it will break more people than it should.
So, I think this PR should not be merged.

wight554 · 2022-05-25T19:35:41Z

Not sure which TypeScript update make it not work. But it will break more people than it should. So, I think this PR should not be merged.

Can you give more context on "breaking"? It passes the test suite, so everything should be fine
Also codebase itself is untouched

climba03003 · 2022-05-25T19:48:47Z

Not sure which TypeScript update make it not work. But it will break more people than it should. So, I think this PR should not be merged.

Can you give more context on "breaking"? It passes the test suite, so everything should be fine Also codebase itself is untouched

I would like to see a repro for this error first.

// tsconfig.json
{
  "compilerOptions": {
    "module": "NodeNext"
  }
}

// index.ts
import defaultFastifyCookie, { fastifyCookie } from "@fastify/cookie";
import Fastify from "fastify";

const fastify = Fastify();
fastify.register(fastifyCookie);
fastify.register(defaultFastifyCookie);

// package.json
{
  "name": "test",
  "version": "1.0.0",
  "scripts": {},
  "dependencies": {
    "@fastify/cookie": "^6.0.0",
    "@types/node": "^17.0.35",
    "fastify": "^4.0.0-rc.2",
    "typescript": "^4.7.2"
  }
}

From what I have tested, I do not face any error currently.

It is working correctly.

wight554 · 2022-05-25T19:50:32Z

Not sure which TypeScript update make it not work. But it will break more people than it should. So, I think this PR should not be merged.

Can you give more context on "breaking"? It passes the test suite, so everything should be fine Also codebase itself is untouched

I would like to see a repro for this error first.
// tsconfig.json
{
  "compilerOptions": {
    "module": "NodeNext"
  }
}

// index.ts
import defaultFastifyCookie, { fastifyCookie } from "@fastify/cookie";
import Fastify from "fastify";

const fastify = Fastify();
fastify.register(fastifyCookie);
fastify.register(defaultFastifyCookie);

// package.json
{
  "name": "test",
  "version": "1.0.0",
  "scripts": {},
  "dependencies": {
    "@fastify/cookie": "^6.0.0",
    "@types/node": "^17.0.35",
    "fastify": "^4.0.0-rc.2",
    "typescript": "^4.7.2"
  }
}
From what I have tested, I do not face any error currently. It is working correctly.

sure
a bit more than this:

{
    "module": "NodeNext",
    "moduleResolution": "NodeNext",
 }

wight554 · 2022-05-25T19:53:16Z

also "type": "module", in package.json
if you investigate further, you'll see that using default import breaks typescript (default import is not callable) and using named import breaks node runtime after successful compilation:

SyntaxError: Named export 'fastifyCookie' not found. The requested module 'fastify-cookie' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:
import pkg from 'fastify-cookie';
const {fastifyCookie} = pkg;

climba03003 · 2022-05-25T19:56:33Z

The problem actually comes from
"type": "module" with "module": "NodeNext"

wight554 · 2022-05-25T20:02:18Z

it seems to be expected behaviour from TS side, it is CJS module so it should use CJS export
you can find more investigation in this issue: bendrucker/snakecase-keys#72

plugin.d.ts

wight554 · 2022-05-25T20:32:28Z

@climba03003 pushed changes to fix default export function type

mcollina

Can you please add a unit test? we use tsd to test our types

wight554 · 2022-05-26T08:18:53Z

Can you please add a unit test? we use tsd to test our types

Idk what can also be tested, current test suite looks completed to me

…to nodenext-export

climba03003

I will not block this PR to be merged and I would like to see the namespace written in PascalCase.

@fastify/typescript just ping more people in to decide what's the next steps.

wight554 · 2022-07-16T12:54:39Z

I will not block this PR to be merged and I would like to see the namespace written in PascalCase.

@fastify/typescript just ping more people in to decide what's the next steps.

PascalCase sounds good to me, will add corresponding changes

wight554 · 2022-07-16T15:33:06Z

Ah well actually @climba03003 PascalCase is no go here. fastifyCookie namespace is merged with fastifyCookie function, and PascalCase for function name doesn't sound great
See https://www.typescriptlang.org/docs/handbook/declaration-merging.html#merging-namespaces-with-classes

climba03003 · 2022-07-16T16:28:28Z

PascalCase is no go here

Sad for this info.

Not blocking for land

RafaelGSS

Could you include a test for ESM? Reference: https://github.com/fastify/fastify/tree/main/test/esm

RafaelGSS · 2022-07-16T20:01:35Z

plugin.d.ts

+import { FastifyPluginCallback } from "fastify";

-declare module 'fastify' {
+declare module "fastify" {


can you restore it? It seems unrelated to the PR goal.

tbh I think it's fine to keep those, that's was formatter does for me every time I touch that file, also makes it consistent with other plugins (at least fastify-static)

wight554 · 2022-07-16T20:08:28Z

Could you include a test for ESM? Reference: https://github.com/fastify/fastify/tree/main/test/esm

I don't really see any reason to test esm since module is CJS only, these changes make typescript read module as CJS

RafaelGSS · 2022-07-16T20:11:01Z

Could you include a test for ESM? Reference: https://github.com/fastify/fastify/tree/main/test/esm

I don't really see any reason to test esm since module is CJS only, these changes make typescript read module as CJS

If the current test suite was any good, it should have detected this problem, shouldn't it? I'm just concerned about preventing a potential regression in any further PR.

wight554 · 2022-07-16T20:14:30Z

Could you include a test for ESM? Reference: https://github.com/fastify/fastify/tree/main/test/esm

I don't really see any reason to test esm since module is CJS only, these changes make typescript read module as CJS

If the current test suite was any good, it should have detected this problem, shouldn't it? I'm just concerned about preventing a potential regression in any further PR.

I think there is no regression as long as it passes current tsd suite, not sure what I can do to test esm in this plugin since there is no esm (.mjs) module exported

wight554 · 2022-07-16T20:27:35Z

Actually there's a regression already since 8da8fbe was added
types don't expect these lines to be inside of fastifyCookie named export

mcollina

lgtm

mcollina · 2022-07-21T14:45:26Z

@wight554 could you rebase?
@RafaelGSS ptal

RafaelGSS

LGTM.

…to nodenext-export

wight554 · 2022-07-21T19:09:20Z

@wight554 could you rebase? @RafaelGSS ptal

should be g2g

* make type definitons `"module": "nodenext"` compatible * see microsoft/TypeScript#48845 * fix test * fix default export type * make typing extra safe and small refactoring * restore brackets * add additional properties to named export and fix test suite * remove formatting * revert formatting * reuse type

* integrate cookie signer * add algorithm as plugin-option * check for matching error message * improve tests * remove unnecessary array conversion * simplify * fix comment * restructure SignerFactory * update typings * fix exports * add benchmarks * improve perf for multi secrets * rename SignerFactory to Signer * Merge branch 'master' into integrate-cookie-signer * export signerFactory * Update signer.js * add secure: 'auto' Option (#199) * add secure: auto Option * document deviation from cookie serialize * describe better * lowercase Lax * Bumped v7.3.0 Signed-off-by: Matteo Collina <hello@matteocollina.com> * make type definitons `"module": "nodenext"` compatible (#184) * make type definitons `"module": "nodenext"` compatible * see microsoft/TypeScript#48845 * fix test * fix default export type * make typing extra safe and small refactoring * restore brackets * add additional properties to named export and fix test suite * remove formatting * revert formatting * reuse type * Bumped v7.3.1 Signed-off-by: Matteo Collina <hello@matteocollina.com> * modify types * remove dummy * fix regex bottleneck found by sonarqube Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Volodymyr Zhdanov <wight554@gmail.com>

wight554 added 2 commits May 25, 2022 21:04

make type definitons "module": "nodenext" compatible

501d0be

* see microsoft/TypeScript#48845

fix test

f286560

Fdawgs requested a review from a team May 25, 2022 18:29

climba03003 previously requested changes May 25, 2022

View reviewed changes

climba03003 reviewed May 25, 2022

View reviewed changes

plugin.d.ts Outdated Show resolved Hide resolved

fix default export type

e82d605

make typing extra safe and small refactoring

f056bc1

mcollina requested changes May 26, 2022

View reviewed changes

Merge branch 'master' of https://github.com/fastify/fastify-cookie in…

4d76e39

…to nodenext-export

wight554 requested review from climba03003 and mcollina July 16, 2022 09:06

climba03003 reviewed Jul 16, 2022

View reviewed changes

RafaelGSS reviewed Jul 16, 2022

View reviewed changes

restore brackets

305bd9b

wight554 added 2 commits July 16, 2022 23:32

add additional properties to named export and fix test suite

b9ba5de

remove formatting

34e48a9

wight554 requested a review from RafaelGSS July 17, 2022 14:22

mcollina approved these changes Jul 18, 2022

View reviewed changes

RafaelGSS approved these changes Jul 21, 2022

View reviewed changes

wight554 added 2 commits July 21, 2022 22:05

Merge branch 'master' of https://github.com/fastify/fastify-cookie in…

c083f79

…to nodenext-export

revert formatting

38df18c

reuse type

da614e9

mcollina merged commit 928f17e into fastify:master Jul 22, 2022

climba03003 mentioned this pull request Sep 1, 2022

typescript doc bug change "import fastify from 'fastify'" to "import { fastify } from 'fastify'" fastify/fastify#4241

Closed

2 tasks

This was referenced Oct 17, 2022

types: Export types in a way compatible with TypeScript "moduleResolution": "NodeNext" fastify/fastify-plugin#200

Closed

Migration of the infamous triplet to next level, Nodenext support fastify/fastify#4349

Closed

Uh oh!

Conversation

wight554 commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

climba03003 left a comment

Choose a reason for hiding this comment

Uh oh!

wight554 commented May 25, 2022

Uh oh!

climba03003 commented May 25, 2022

Uh oh!

wight554 commented May 25, 2022

Uh oh!

wight554 commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

climba03003 commented May 25, 2022

Uh oh!

wight554 commented May 25, 2022

Uh oh!

Uh oh!

wight554 commented May 25, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

wight554 commented May 26, 2022

Uh oh!

climba03003 left a comment

Choose a reason for hiding this comment

Uh oh!

wight554 commented Jul 16, 2022

Uh oh!

wight554 commented Jul 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

climba03003 commented Jul 16, 2022

Uh oh!

RafaelGSS left a comment

Choose a reason for hiding this comment

Uh oh!

RafaelGSS Jul 16, 2022

Choose a reason for hiding this comment

Uh oh!

wight554 Jul 16, 2022

Choose a reason for hiding this comment

Uh oh!

wight554 commented Jul 16, 2022

Uh oh!

RafaelGSS commented Jul 16, 2022

Uh oh!

wight554 commented Jul 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wight554 commented Jul 16, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jul 21, 2022

Uh oh!

RafaelGSS left a comment

Choose a reason for hiding this comment

Uh oh!

wight554 commented Jul 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wight554 commented May 25, 2022 •

edited

Loading

wight554 commented May 25, 2022 •

edited

Loading

wight554 commented Jul 16, 2022 •

edited

Loading

wight554 commented Jul 16, 2022 •

edited

Loading