AxaFrance / oidc-client

Light, Secure, Pure Javascript OIDC (Open ID Connect) Client. We provide also a REACT wrapper (compatible NextJS, etc.).
MIT License
597 stars 160 forks source link

npm package must not be shipped with TypeScript source code #854

Closed ryanelian closed 2 years ago

ryanelian commented 2 years ago

Issue and Steps to Reproduce

npm package MUST NOT be shipped with TypeScript source code and instead ship a bundled JS file and TypeScript definition files instead (.d.ts files)

This is important because importing the module from the package causes the TypeScript compiler to link against the source code inside the npm package, which is not desirable:

import { OidcProvider } from '@axa-fr/react-oidc';

image

Which causes the project to pick up errors from transitive dependency:

image

Notice that the TypeScript compile errors came from the @openid/appauth project, not the react-oidc code itself.

Versions

6.5.5

Screenshots

Provided above

Expected

Importing from react-oidc project resolves to the TypeScript definition files (.d.ts) not the .ts source code themselves

Actual

Importing from react-oidc project resolves to the .ts source code of the project itself, which is not desirable

Additional Details

A library author might have a faulty import or might only provide typescript files for his library and not transpiled code.

libraries shouldn't publish *.ts files,

https://github.com/microsoft/TypeScript/issues/40426

TypeScript 4.8.3

guillaume-chervet commented 2 years ago

Thank you very much @ryanelian for these rich issue and bringing all that information.

I have to study a little bit how to resolve this.

ryanelian commented 2 years ago

Thank you very much @ryanelian for these rich issue and bringing all that information.

You're welcome! Thanks for developing such a nice library.

Our company is integrating this library into our Next.js new project template so we'll probably find more issues down the line.

I have to study a little bit how to resolve this.

image

image

Reference: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

image

Then the package should be created without TypeScript source code and purely just JS and declaration files.


Sample package created:

axa-fr-react-oidc-6.5.5.tgz.zip

ryanelian commented 2 years ago

Also, something kind of related to producing JS output for the package...

image

If you do this, the package would be optimized for build systems (since almost everyone are writing React codes using Babel / Next.js / webpack / some kind of build tools) anyway.

ES2015 output will reduce the package size. Babel / etc. usually consume them to produce ES5 codes if needed anyway. You can even probably dial that up to ES2017 so that async codes are preserved (so that TypeScript won't generate awaiter function on every files, saving more file size).

ESNext module will ensure that bundlers such as webpack / etc. can perform tree-shaking optimization (removing codes that aren't really used).

I recommend using these options instead of ES5 + CommonJS.

guillaume-chervet commented 2 years ago

hi @ryanelian, it seems too work pretty well thank you very much :)

ryanelian commented 2 years ago

hi @ryanelian, it seems too work pretty well thank you very much :)

Hi again. Before I get mass-lynched by the other users of the library, I think I need to apologize πŸ˜…

The part of changes where you don't ship the TypeScript code works brilliantly. So there's one problem out of the way. πŸ‘


However... The part of the changes where the tsconfig.json module is changed from CommonJS to ESNext...

βœ… Does not show error in VS Code / tsc βœ… Does not show error when the Next.js is being run βœ… Does not show error when the OIDC component was loaded on browser-side only (when using Next.js dynamic ssr=false) β˜’οΈβ˜ β— Explodes magnificently when the OIDC component is loaded on the server-side (SSR)

[dev:nextjs] Error [ERR_REQUIRE_ESM]: require() of ES Module D:\VS\accelist-nextjs-starter\node_modules\@axa-fr\react-oidc\dist\index.js from D:\VS\accelist-nextjs-starter\.next\server\pages\_app.js not supported.
[dev:nextjs] Instead change the require of index.js in D:\VS\accelist-nextjs-starter\.next\server\pages\_app.js to a dynamic import() which is available in all CommonJS modules.
[dev:nextjs]     at Object.@axa-fr/react-oidc (D:\VS\accelist-nextjs-starter\.next\server\pages\_app.js:140:18)
[dev:nextjs]     at __webpack_require__ (D:\VS\accelist-nextjs-starter\.next\server\webpack-runtime.js:33:42)
[dev:nextjs]     at eval (webpack-internal:///./components/NextJsOidcProvider.tsx:9:76)
[dev:nextjs]     at Object../components/NextJsOidcProvider.tsx (D:\VS\accelist-nextjs-starter\.next\server\pages\_app.js:65:1)
[dev:nextjs]     at __webpack_require__ (D:\VS\accelist-nextjs-starter\.next\server\webpack-runtime.js:33:42)
[dev:nextjs]     at eval (webpack-internal:///./pages/_app.tsx:18:88)
[dev:nextjs]     at Function.__webpack_require__.a (D:\VS\accelist-nextjs-starter\.next\server\webpack-runtime.js:97:13)
[dev:nextjs]     at eval (webpack-internal:///./pages/_app.tsx:1:21)
[dev:nextjs]     at Object../pages/_app.tsx (D:\VS\accelist-nextjs-starter\.next\server\pages\_app.js:109:1)
[dev:nextjs]     at __webpack_require__ (D:\VS\accelist-nextjs-starter\.next\server\webpack-runtime.js:33:42)
[dev:nextjs]     at __webpack_exec__ (D:\VS\accelist-nextjs-starter\.next\server\pages\_app.js:238:39)
[dev:nextjs]     at D:\VS\accelist-nextjs-starter\.next\server\pages\_app.js:239:28
[dev:nextjs]     at Object.<anonymous> (D:\VS\accelist-nextjs-starter\.next\server\pages\_app.js:242:3)
[dev:nextjs]     at Object.requirePage (D:\VS\accelist-nextjs-starter\node_modules\next\dist\server\require.js:58:12)
[dev:nextjs]     at D:\VS\accelist-nextjs-starter\node_modules\next\dist\server\load-components.js:57:54
[dev:nextjs]     at runMicrotasks (<anonymous>)
[dev:nextjs]     at async Promise.all (index 1)
[dev:nextjs]     at async Object.loadComponents (D:\VS\accelist-nextjs-starter\node_modules\next\dist\server\load-components.js:55:33)
[dev:nextjs]     at async DevServer.findPageComponents (D:\VS\accelist-nextjs-starter\node_modules\next\dist\server\next-server.js:545:36) {
[dev:nextjs]   code: 'ERR_REQUIRE_ESM'
[dev:nextjs] }

So you should probably revert the part where you modified the tsconfig.json module back to CommonJS

Apparently the server-side of the Next.js still imports using require unlike the browser-side which supports ES Module. The ES2015 target works just fine though.

I apologize for not testing the change I suggested thoroughly and I have no excuse. πŸ™

guillaume-chervet commented 2 years ago

Thank you again @ryanelian for your awesome feedback. I have revert to CommonJS and published a fixed version.