expose named export #231

akphi · 2022-10-11T16:55:08Z

Checklist

I'm not as ambitious as #224, I'm just trying to make sure named exports work for @fastify/cors. I'm following what we already did in https://github.com/fastify/fastify

https://github.com/fastify/fastify/blob/acba25acc3e97e3598daf79537a39a5272dba7b8/fastify.js#L783-L796

/**
 * These export configurations enable JS and TS developers
 * to consumer fastify in whatever way best suits their needs.
 * Some examples of supported import syntax includes:
 * - `const fastify = require('fastify')`
 * - `const { fastify } = require('fastify')`
 * - `import * as Fastify from 'fastify'`
 * - `import { fastify, TSC_definition } from 'fastify'`
 * - `import fastify from 'fastify'`
 * - `import fastify, { TSC_definition } from 'fastify'`
 */
module.exports = fastify
module.exports.fastify = fastify
module.exports.default = fastify

run npm run test and npm run lint
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

Uzlopak

I dont want to close it without discussion but also dont want that somebody merges this without thinking the implications.

This plugin uses fastify-plugin. Fastify-plugin itself provides a named export.

So you can do

const { fastifyCors } = require('@fastify/cors')

So i would not merge this, as we should already provide a named export.

Uzlopak · 2022-10-11T17:36:20Z

See https://github.com/fastify/fastify-plugin/blob/35a6288314ecdc610429c5675f01b4b453279796/plugin.js#L61

kibertoad · 2022-10-11T17:36:26Z

@Uzlopak we should at least adjust TS types and readme to indicate that is possible then

Uzlopak · 2022-10-11T17:38:50Z

If you want to earn some brownie points than it would be good if we can fix the typings so that latest tsd passes and nodenext is supported. See the other open PR.

:)

Uzlopak · 2022-10-11T17:40:56Z

Ah wait. I thought @akphi answered...

But well. Still the same. We should fix nodenext

akphi · 2022-10-11T18:36:54Z

@Uzlopak what do you suppose we need for nodenext though? I'm not aware of the js part, so now you said it, I could omit my changes for index.js and just add the named export to index.d.ts.

I have tried locally to hack the index.d.ts to do a named export for fastifyCors, but I got the following error still

import { fastifyCors } from '@fastify/cors';
         ^^^^^^^^^^^
SyntaxError: Named export 'fastifyCors' not found. The requested module '@fastify/cors' 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:

that makes me believe my change to the index.js file was appropriate (and yes, I was using NodeNext)

Uzlopak · 2022-10-11T19:33:18Z

If you dont want to help to get #224 done, it is ok. Yes, just do the typescrpt change and also add a typing test :)

akphi · 2022-10-11T20:35:16Z

@Uzlopak hm, compared to #230 I'm trying to make this work with the named import case for NodeNext, but you can see my repro here

akphi/issue-repo#11

Even after patching the index.d.ts to support named export, i.e.

export { fastifyCors };

it doesn't work, I got the error like mentioned in that PR:

import { fastifyCors } from "@fastify/cors";
         ^^^^^^^^^^^
SyntaxError: Named export 'fastifyCors' not found. The requested module '@fastify/cors' 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/cors';
const { fastifyCors } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:123:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:189:5)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:533:24)
    at async loadESM (node:internal/process/esm_loader:91:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12)

I believe I still need to make the changes I made in index.js

Uzlopak

Just a remark

Uzlopak · 2022-10-12T15:15:01Z

index.js

@@ -220,7 +220,15 @@ function isRequestOriginAllowed (reqOrigin, allowedOrigin) {
  }
 }

-module.exports = fp(fastifyCors, {
+const _fastifyCors = fp(fastifyCors, {


revert tiese changes, and I will approve and merge it ;)

This addresses the issue I found in akphi/issue-repo#11 in my project using NodeNext. Is there something I miss here, but what you said about fastify-plugin just doesn't work for our Typescript project. I can help investigate, but I believe this is the fix

index.js

Uzlopak · 2022-10-13T08:44:30Z

Do we have an underlying issue in fastify-plugin?

mcollina

lgtm

Uzlopak · 2022-10-13T12:37:53Z

@mcollina

We should not merge this.

Eomm · 2022-10-13T17:04:57Z

We should not merge this.

Please, use the "Request changes" review to block or I may miss it 😓

Do you think the named export could be added to fastify-plugin as a feature?

Uzlopak · 2022-10-13T17:17:08Z

I already did use Request Changes option.

We have already named export in fastify-plugin. So the question is, if it is actually properly working or not. I have the feeling that the issue is typescript and not Javascript.

akphi · 2022-10-13T17:45:26Z

I'll try to setup just a JS script with ESmoule import instead of require to check if it's a Typescript thing or not

Eomm · 2022-10-13T18:22:36Z

We have already named export in fastify-plugin.

Could you link it?

Uzlopak · 2022-10-13T18:28:36Z

Named Export, e.g. const fastifyCors = require('@fastify/cors').fastifyCors
https://github.com/fastify/fastify-plugin/blob/bdbe525a949e34f1fb9244e33b9d210ca0fe2b39/plugin.js#L58

.default Export, e.g. const fastifyCors = require('@fastify/cors').default
https://github.com/fastify/fastify-plugin/blob/bdbe525a949e34f1fb9244e33b9d210ca0fe2b39/plugin.js#L50

Default Export, e.g. const fastifyCors = require('@fastify/cors')
https://github.com/fastify/fastify-plugin/blob/bdbe525a949e34f1fb9244e33b9d210ca0fe2b39/plugin.js#L61

akphi · 2022-10-14T03:12:01Z

I tested with akphi/issue-repo#11

import { fastify } from "fastify";
import { fastifyCors } from "@fastify/cors";

const PORT = 9999;

const server = fastify({
  logger: true,
});

server.register(fastifyCors, {
  methods: ["OPTIONS"],
  origin: [/localhost/],
  credentials: true,
});

server.listen(
  {
    port: PORT,
  },
  (error, address) => {
    if (error) {
      throw error;
    }
  }
);

If you run the script yarn start:js you would get the error

import { fastifyCors } from "@fastify/cors";
         ^^^^^^^^^^^
SyntaxError: Named export 'fastifyCors' not found. The requested module '@fastify/cors' 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/cors';
const { fastifyCors } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:123:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:189:5)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:533:24)
    at async loadESM (node:internal/process/esm_loader:91:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12)

I have tested by poking into node_modules/@fastify/cors/index.js and made the changes I made in this PR and things work fine. So this is not a problem just pertaining to TS.

climba03003 · 2022-10-14T05:16:08Z

So this is not a problem just pertaining to TS.

It is a TS problem, because TS will read the file and see if it suitable.
TS is not smart enough to see fastify-plugin syntax to be identical to module.export.<name> = <func>.

Of cause, the changes in index.d.ts is needed.

akphi · 2022-10-14T05:39:27Z

So this is not a problem just pertaining to TS.

It is a TS problem, because TS will read the file and see if it suitable. TS is not smart enough to see fastify-plugin syntax to be identical to module.export.<name> = <func>.

Of cause, the changes in index.d.ts is needed.

But I faced the problem when I run node index.js in that project, I don't get which part involves TS here, did I miss something?

climba03003 · 2022-10-14T06:10:59Z

So this is not a problem just pertaining to TS.

It is a TS problem, because TS will read the file and see if it suitable. TS is not smart enough to see fastify-plugin syntax to be identical to module.export.<name> = <func>.

Of cause, the changes in index.d.ts is needed.

But I faced the problem when I run node index.js in that project, I don't get which part involves TS here, did I miss something?

Strange, it certainly something changes in node. From reading the document, it's seems like the current import of commonjs will go through a static analysis and it only permit some of the syntax.

Eomm · 2022-10-14T06:46:52Z

Yes, something strange is happening here.
I replicated the issue and if you try:

const awaited = await import( "@fastify/cors");
console.log({awaited})

const { fastifyCors } = awaited.default;
console.log(fastifyCors);

YOu will see that there is an unwanted and additional default wrapper

climba03003 · 2022-10-14T06:53:09Z

Yes, something strange is happening here.

In the latest version of node document. import default now directly map to module.exports which means unless the static analysis think that it should be a named export. There is no way to provide dynamic named export like before.

The ECMAScript Module Namespace representation of a CommonJS module is always a namespace with a default export key pointing to the CommonJS module.exports value.

akphi · 2022-10-15T13:55:59Z

@climba03003 does this justify my fix? Is there anything else you need?

Uzlopak · 2022-10-16T14:55:36Z

Sry

climba03003 · 2022-10-17T06:35:31Z

Don't we generally use "the infamous triplet" across organization, similarly how it was done in the original version of this PR, to address this issue?

Yes, but the current node using static analysis when CJS is imported from ESM. Which means the current fastify-plugin no longer works for the "the infamous triplet".
We need to explicit use module.exports in all the plugin if we want to support both CJS and ESM. I don't know which version update change this behavior since TypeScript actually helps me to eliminate the import problem. But the truth is that currently only explicit module.exports will works.

fox1t · 2022-10-17T09:54:38Z

OK, the issue is now clear to me. Let me try to summarise it. When Node.js uses ESM context (so you are running an entry point with .mjs extension or have "type": "module" in your package.json) when you import a CJS package.json, Node.js will treat it as cjs regardless of the exports we provide and types we add, and it refuses to import a named export. I think that as we found the "famous triplet," we should try to find a superset of it to support also "type": "module" without losing all of the benefits that we already have. Not everyone is ready to go full ESM yet.

climba03003 · 2022-10-17T10:00:26Z

Node.js will treat it as cjs regardless of the exports we provide and types we add, and it refuses to import a named export.

It will support if you explicit specify in module.exports.
Currently, we use fastify-plugin as dynamic generation of named export which is not supported by node.

// supported in NodeJS and TypeScript
module.exports = plugin
module.exports.default = plugin // here may not be needed, but maybe TypeScript need it
module.exports.plugin = plugin

// only supported in TypeScript
module.exports = fp(plugin)

I have tried various old version of node including 12, 14, 16, 18.0.0. Non of them actually support the dynamic generation of named export.

fox1t · 2022-10-17T10:08:21Z

Yes! I've just played a little bit with a repro on my machine and we need to rely on that Node.js static analysis on CJS imports. Are we saying that we should "just" fix fp here?

climba03003 · 2022-10-17T10:17:44Z

Are we saying that we should "just" fix fp here?

@fox1t

They are different problem. In here, we need to support native nodejs, so we must use module.exports.
In fastify-plugin, it is the problem of how TypeScript resolving modules.

TypeScript resolve module based on types and NodeJS based on static analysis of JavaScript file.
Both patch are needed in their own aspect.

fox1t · 2022-10-17T10:23:08Z

Yes, I am aware of it. Probably I've written the reply too fast and with few details. First, we should add explicit triplets to fastify the plugins we maintain to make Node.js' ESM context happy while leaving the TS exports as ESM triplets to make TS happy.
Then we should patch FP package itself. Am I missing something?

climba03003 · 2022-10-17T10:28:56Z

First, we should add explicit triplets to fastify the plugins we maintain to make Node.js' ESM context happy while leaving the TS exports as ESM triplets to make TS happy.
Then we should patch FP package itself. Am I missing something?

Nothing missing.

For a complete support for all combination of both NodeJS and TypeScript in ESM and CJS.
The format of plugin and typings should be updated as follow.

// index.js

function plugin() {}

// CJS export
module.exports = plugin
// TypeScript ESM default export, not confirmed
module.exports.default = plugin
// CJS and ESM named export
module.exports.plugin = plugin

// index.d.ts

// type for plugin
type Plugin = FastifyPluginCallback<plugin.PluginOptions>

// required for CJS
namespace plugin {
  // other export of typings
  export interface PluginOptions {}

  // CJS and ESM named export
  export const plugin: Plugin
  // ESM default export
  export { default as plugin }
}

// actual plugin usage
declare function plugin(...params: Parameters<Plugin>): : ReturnType<Plugin>

// CJS module.exports
export = plugin

fox1t · 2022-10-17T10:35:03Z

I am still not convinced about export = plugin, as I pointed out in fastify/fastify-plugin#200 (comment)

Exporting namespace will break "moduleInterop": false compilation environments and force them to use import star for "default" imports.

climba03003 · 2022-10-17T10:38:52Z

Exporting namespace will break "moduleInterop": false compilation environments and force them to use import star for "default" imports.

No, it is supported by module.exports.default.
https://www.typescriptlang.org/tsconfig#esModuleInterop
You can check the second point of esModuleInterop

import moment from "moment" acts the same as const moment = require("moment").default

fox1t · 2022-10-17T10:42:27Z

I am talking about the types here (so what the compiler sees thanks to the type declarations), not the runtime (so module.exports.default). I would like to see a repro that passes all of the cases we support usual when using export =. :/

climba03003 · 2022-10-17T10:46:11Z

I am talking about the types here (so what the compiler sees thanks to the type declarations), not the runtime (so module.exports.default). I would like to see a repro that passes all of the cases we support usual when using export =. :/

Yes, it is supported. You can use @fastify/cookie as an example.

// tsconfig.json
{
  "compilerOptions": {
    "esModuleInterop": false
  }
}

// index.ts
import FastifyCookie from '@fastify/cookie';
import Fastify from 'fastify';

const fastify = Fastify()
fastify.register(FastifyCookie)

;(async function() {
  await fastify.ready()
})()

fox1t · 2022-10-17T10:49:34Z

@climba03003 awesome. We should write up somewhere all of these findings to remember them in the future!

fox1t · 2022-10-17T10:59:58Z

@climba03003 In order to make it work, I needed to slightly adjust your types:

type Plugin = FastifyPluginCallback<plugin.PluginOptions>

// required for CJS
declare namespace plugin { // <- added declare
  // other export of typings
  export interface PluginOptions {}

  // CJS and ESM named export
  export const plugin: Plugin
  // ESM default export
  export { plugin as default } // <- instead of export { default as plugin } 
}

// actual plugin usage
declare function plugin(...params: Parameters<Plugin>): ReturnType<Plugin>

// CJS module.exports
export = plugin

fox1t · 2022-10-17T11:01:17Z

This is the explanation of what happened here (and in other Fastify packages) and why we need to start using the above types in the future!

microsoft/TypeScript#50690 (comment)

climba03003 · 2022-10-17T11:11:59Z

This is the explanation of what happened here (and in other Fastify packages) and why we need to start using the above types in the future!

microsoft/TypeScript#50690 (comment)

Issue has been open in fastify/fastify#4349

Uzlopak · 2022-10-17T12:36:12Z

Two remarks:

Should we add back
module.exports.default = _fastifyCors
?
I fixed fix(types): make definitions nodenext compatible #224

Uzlopak · 2022-10-24T09:17:04Z

Is this still relevant afterr #224 ?

climba03003 · 2022-10-24T09:17:38Z

Is this still relevant afterr #224 ?

Yes, keeps the changes inside index.js and the TypeScript test.

akphi · 2022-10-24T13:55:28Z

@climba03003 I have updated the PR

I am not convinced but I dont want to be the blocker

Uzlopak requested changes Oct 11, 2022

View reviewed changes

Uzlopak previously requested changes Oct 12, 2022

View reviewed changes

Eomm reviewed Oct 12, 2022

View reviewed changes

index.js Show resolved Hide resolved

mcollina approved these changes Oct 13, 2022

View reviewed changes

Eomm approved these changes Oct 16, 2022

View reviewed changes

Uzlopak closed this Oct 16, 2022

Uzlopak reopened this Oct 16, 2022

climba03003 approved these changes Oct 16, 2022

View reviewed changes

climba03003 mentioned this pull request Oct 17, 2022

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

Closed

4 tasks

climba03003 mentioned this pull request Oct 17, 2022

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

Closed

2 tasks

akphi added 3 commits October 24, 2022 09:54

expose named export

61db326

minor lint fixes

7294e33

added test for TS named export

eac8499

akphi force-pushed the nodenext branch from f4abd3b to eac8499 Compare October 24, 2022 13:54

climba03003 approved these changes Oct 24, 2022

View reviewed changes

mcollina merged commit 1d09522 into fastify:master Oct 25, 2022

akphi deleted the nodenext branch October 25, 2022 14:40

akphi mentioned this pull request Oct 26, 2022

minor bug fixes finos/legend-studio#1573

Merged

13 tasks

Uh oh!

expose named export #231

expose named export #231

Uh oh!

Conversation

akphi commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uzlopak left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Oct 11, 2022

Uh oh!

kibertoad commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uzlopak commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uzlopak commented Oct 11, 2022

Uh oh!

akphi commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uzlopak commented Oct 11, 2022

Uh oh!

akphi commented Oct 11, 2022

Uh oh!

Uzlopak left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uzlopak Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

akphi Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uzlopak commented Oct 13, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Oct 13, 2022

Uh oh!

Eomm commented Oct 13, 2022

Uh oh!

Uzlopak commented Oct 13, 2022

Uh oh!

akphi commented Oct 13, 2022

Uh oh!

Eomm commented Oct 13, 2022

Uh oh!

Uzlopak commented Oct 13, 2022

Uh oh!

akphi commented Oct 14, 2022

Uh oh!

climba03003 commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akphi commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

climba03003 commented Oct 14, 2022

Uh oh!

Eomm commented Oct 14, 2022

Uh oh!

climba03003 commented Oct 14, 2022

Uh oh!

akphi commented Oct 15, 2022

Uh oh!

Uzlopak commented Oct 16, 2022

Uh oh!

climba03003 commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fox1t commented Oct 17, 2022

akphi commented Oct 11, 2022 •

edited

Loading

kibertoad commented Oct 11, 2022 •

edited

Loading

Uzlopak commented Oct 11, 2022 •

edited

Loading

akphi commented Oct 11, 2022 •

edited

Loading

Uzlopak left a comment •

edited

Loading

akphi Oct 12, 2022 •

edited

Loading

climba03003 commented Oct 14, 2022 •

edited

Loading

akphi commented Oct 14, 2022 •

edited

Loading

climba03003 commented Oct 17, 2022 •

edited

Loading

climba03003 commented Oct 17, 2022 •

edited

Loading

climba03003 commented Oct 17, 2022 •

edited

Loading

climba03003 commented Oct 17, 2022 •

edited

Loading

climba03003 commented Oct 17, 2022 •

edited

Loading

fox1t commented Oct 17, 2022 •

edited

Loading

climba03003 commented Oct 17, 2022 •

edited

Loading

Uzlopak commented Oct 17, 2022 •

edited

Loading

climba03003 commented Oct 24, 2022 •

edited

Loading