fix(NODE-5873): objectId symbol property not defined on instances from cross cjs and mjs

[dot-i]

Description

Fix interoperability of bson between ESM and CommonJS.

What is changing?

Symbol('id') is replaced with Symbol.for('@@mdb.bson.id') in objectid.ts. This avoids the issue of two different symbols being used for ESM and CommonJS, causing ObjectId.equals() to fail with an undefined error.

Is there new documentation needed for these changes?

No.

What is the motivation for this change?

We are switching our modules to be ESM and ran in to this issue because we imported bson directly from our module (ESM), but also sometimes through a CommonJS import which in turn also imported bson -- but as CommonJS module.

This caused ObjectId.equals() to fail since there were two separate 'id' symbols in use, causing an undefined error.

Release Highlight

Fixed objectId symbol property not defined on instances from cross cjs and mjs

We do recommend that users of the driver use the BSON APIs exported from the driver. One reason for this is at this time the driver is only shipped in commonjs format and as a result it will only import the commonjs BSON bundle. If in your application you use import syntax then there will be a commonjs and an es module instance in the current process which prevents things like instanceof from working.

Also, private symbols defined in one package will not be equal to symbols defined in the other. This caused an issue on ObjectId's private symbol property preventing the .equals method from one package from operating on an ObjectId created from another.

Thanks to @dot-i's contribution we've changed the private symbol to a private string property so that the .equals() method works across module types.

Double check the following

• Ran npm run check:lint script
• Self-review completed using the steps outlined here
• PR title follows the correct format: type(NODE-xxxx)[!]: description
  • Example: feat(NODE-1234)!: rewriting everything in coffeescript
• Changes are covered by tests
• New TODOs have a related JIRA ticket

[durran]

@dot-i can you also run the tests as well and fix any failures? Example:

[2024/01/31 21:04:34.125] > npm run check:node && npm run check:web && npm run check:web-no-bigint
[2024/01/31 21:04:34.903] created lib/bson.rn.cjs in 654ms
[2024/01/31 21:04:34.903] { web: false, noBigInt: false, nodeVersion: 'v20.11.0' }
[2024/01/31 21:04:36.187] Error: Did not find Symbol(id) on 6b61666665656b6c61746368
[2024/01/31 21:04:36.187]     at getSymbolFrom (/data/mci/db7d87dd5c732b6aca749a47f85e508c/src/test/node/tools/utils.js:200:11)
[2024/01/31 21:04:36.187]     at Suite.<anonymous> (/data/mci/db7d87dd5c732b6aca749a47f85e508c/src/test/node/object_id.test.ts:379:33)

[dot-i]

@dot-i can you also run the tests as well and fix any failures?

@durran Fixed!

[addaleax]

Fwiw I'm good with this change 👍

[nbbeeken]

@durran @addaleax Should we consider changing the symbol property to a string one? I don't see a motive for it anymore, combined with registering globally it doesn't seem to simulate "private" any more than typescript private does.

[dot-i]

@durran @addaleax Should we consider changing the symbol property to a string one? I don't see a motive for it anymore, combined with registering globally it doesn't seem to simulate "private" any more than typescript private does.

Anyone know what the original motive was for using a symbol?

[nbbeeken]

Apparently, I was the one who introduced it 😅 #394.

At the time we were converting from JS to TS and in JS we felt that class property "privacy" was best encoded by attaching things as symbols and I was just following the pattern started before me in the changes necessary for that PR. Back then in the ancient era of ~3.5yrs ago, we didn't need to think of dual module hazards.

I think to solve the ESM and cross-version issue we have today there's nothing wrong with going back to a string property and using Typescript to indicate privacy.

This is the only Symbol property we have in BSON, so it is already an odd one out in that sense.

[dot-i]

@nbbeeken what string property name would you propose?

  get id(): Uint8Array {
    return this[kId];
  }

Since there's this getter, a simple 'id' is not possible. And there's already a __id property for the hexString cache.

[nbbeeken]

How about buffer? taking the idea from our Binary type, but it should be private here on the ObjectId.

[dot-i]

How about buffer? taking the idea from our Binary type, but it should be private here on the ObjectId.

Works for me 👍

Do I update the PR to use a #buffer property instead of the symbol?

Or maybe a simple private buffer would suffice as #buffer would lead to the use of __classPrivateFieldGet() and __classPrivateFieldSet() helper functions (which would add overhead).

[nbbeeken]

Just private buffer works. And yes, that is exactly for the reason you call out, at this time we're avoiding using #private variables due to the overhead.

[dot-i]

Changed to use private buffer, adjusted unit test accordingly.

[nbbeeken]

LGTM, thanks so much for the help here!