Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dual package hazard #168

Closed
jcbhmr opened this issue May 20, 2023 · 9 comments
Closed

Dual package hazard #168

jcbhmr opened this issue May 20, 2023 · 9 comments

Comments

@jcbhmr
Copy link

jcbhmr commented May 20, 2023

It would appear that this package encounters the dual package hazard with the base64 code being run potentially twice: once when import-ed and once when require()-ed.

https://nodejs.org/api/packages.html#packages_dual_package_hazard

image

This hasn't been an issue for me in practice, but it does happen!

// app.cjs
async function main() {
  const { encode: encode1, decode: decode1 } = require('js-base64')
  const { encode: encode2, decode: decode2 } = await import('js-base64')

  console.log({ encode1, encode2, decode1, decode2 })
  console.log("encode1 === encode2", encode1 === encode2)
  console.log("decode1 === decode2", decode1 === decode2)
}
main();
{
  encode1: [Function: encode],
  encode2: [Function: encode],
  decode1: [Function: decode],
  decode2: [Function: decode]
}
encode1 === encode2 false
decode1 === decode2 false

This isn't terrible, it's just a quirk that should probably be fixed. 😉
(You can close this if this isn't a big deal.)

@dankogai
Copy link
Owner

This is more like a feature than an issue but I'll leave it open so users are aware of such possibilities like this 😉

@jcbhmr
Copy link
Author

jcbhmr commented May 23, 2023

Would it perhaps be useful to add such documentation to the readme? 🤔 I think that might be better than a perpetually open issue 🤣

@dankogai
Copy link
Owner

#172 fixes this problem so closing.

@jcbhmr
Copy link
Author

jcbhmr commented Jan 20, 2024

I don't think so? There's still two copies of the code: one that gets run when you import "js-base64" and one that gets run when you require("js-base64").

@jcbhmr ➜ /workspaces/js-base64 (main) $ node
Welcome to Node.js v20.11.0.
Type ".help" for more information.
> let esm = await import("js-base64")
undefined
> let cjs = require("js-base64")
undefined
> esm
[Module: null prototype] {
  // ...
}
> cjs
{
  // ...
}
> esm.atob === cjs.atob
false
> esm.Base64 === cjs.Base64
false
> 

using commit 53644d0

@jcbhmr
Copy link
Author

jcbhmr commented Jan 20, 2024

The solution is to treat one implementation as "the implementation" (probably the CJS one) and then just re-export that in the ESM version like this:

// base64.mjs
import exports from "./base64.js";

export const {
  version,
  VERSION,
  atob,
  atobPolyfill,
  btoa,
  btoaPolyfill,
  fromBase64,
  toBase64,
  encode,
  encodeURI,
  encodeURL,
  utob,
  btou,
  decode,
  isValid,
  fromUint8Array,
  toUint8Array,
  extendString,
  extendUint8Array,
  extendBuiltins,
  Base64,
} = exports;
export default exports;
@jcbhmr ➜ /workspaces/js-base64 (main) $ node
Welcome to Node.js v20.11.0.
Type ".help" for more information.
> let esm = await import("js-base64")
undefined
> let cjs = require("js-base64")
undefined
> esm.atob === cjs.atob
true
> esm.Base64 === cjs.Base64
true
> 

@dankogai
Copy link
Owner

It is not only okay but necessary for esm.atob === cjs.atob to be false even their contents are identical. Consider:

const funcA = function(){}
const funcB = function(){}
funcA === funcB // false

I don't think so? There's still two copies of the code: one that gets run when you import "js-base64" and one that gets run when you require("js-base64").

@jcbhmr ➜ /workspaces/js-base64 (main) $ node
Welcome to Node.js v20.11.0.
Type ".help" for more information.
> let esm = await import("js-base64")
undefined
> let cjs = require("js-base64")
undefined
> esm
[Module: null prototype] {
  // ...
}
> cjs
{
  // ...
}
> esm.atob === cjs.atob
false
> esm.Base64 === cjs.Base64
false
> 

using commit 53644d0

@jcbhmr
Copy link
Author

jcbhmr commented Jan 20, 2024

const funcA = function(){}
const funcB = function(){}
funcA === funcB // false

You're absolutely right! The functions would be different! And that's actually exactly what's happening. There's two pieces of code running each creating their own function that is therefore NOT esm.theExport === cjs.theExport to the other one. When you import("js-base64") it runs the base64.mjs due to the import condition being satisfied in package.json. Then when you (or one of your dependencies that isn't using ESM yet) uses require("js-base64") it gets a different file: base64.js (CJS) which then executes a different version of the code so that you end up with one execution for the ESM and then a second execution for the CJS.

This rears its head in bundlers since bundlers will then include both the base64.js and the base64.mjs file effectively making the library 2x the size lol.

It can also cause issues with new esm.Class() instanceof cjs.Class returning false when you'd intuitively think it'd return true (not an issue since js-base64 doesn't use classes)

The solution is to delegate to the "canonical" CJS function and wrap it in a nice ESM interface with appropriate named exports. Normally Node.js does this for you using https://github.com/nodejs/cjs-module-lexer to extract the named exports so you can normally do import { hello } from "./x.cjs" but since base64.js doesn't have those tokens you need the .mjs wrapper to explicitly pluck the named exports out of the CJS object.

The official Node.js docs recommend this solution:

Approach # 1: Use an ES module wrapper#

import cjsModule from './index.cjs';
export const name = cjsModule.name;
export default cjsModule; 

https://nodejs.org/api/packages.html#approach-1-use-an-es-module-wrapper

@dankogai
Copy link
Owner

Sure it doubles the size of the library but it is still 8.3kB. It also make both cjs and mjs stand alone which matters a lot especially when you want to use CDN versions like:

<script src="https://cdn.jsdelivr.net/npm/[email protected]/base64.min.js"></script>
npm notice === Tarball Details === 
npm notice name:          js-base64                               
npm notice version:       3.7.6                                   
npm notice filename:      js-base64-3.7.6.tgz                     
npm notice package size:  8.3 kB                                  
npm notice unpacked size: 38.7 kB                                 
npm notice shasum:        6ccb5d761b48381fd819f9ce04998866dbcbbc99
npm notice integrity:     sha512-NPrWuHFxFUknr[...]OgPQ6Zq+EDiTA==
npm notice total files:   7                                       

@jcbhmr
Copy link
Author

jcbhmr commented Jan 20, 2024

Yes, it's minor. I'll drop this since it's OK to duplicate code since you're right it's small enough to not matter 👍

For CDNs you're right that raw esm code is nice. I'm a user of https://esm.sh and https://esm.run (from jsDelivr) so I haven't had reason to use the raw .min.js file directly before 🤷‍♂️

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 a pull request may close this issue.

2 participants