dankogai / js-base64

Base64 implementation for JavaScript
BSD 3-Clause "New" or "Revised" License
4.27k stars 1.33k forks source link

Dual package hazard #168

Closed jcbhmr closed 9 months ago

jcbhmr commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

dankogai commented 9 months ago

https://github.com/dankogai/js-base64/pull/172 fixes this problem so closing.

jcbhmr commented 9 months ago

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 53644d0db19d15b035a32a63f479664e86a33b5b

jcbhmr commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago
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 commented 9 months ago

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/js-base64@3.7.6/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 commented 9 months ago

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 🤷‍♂️