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

feat!: Remove all uses of CJS, add named Flagsmith export #163

Merged
merged 12 commits into from
Nov 7, 2024

Conversation

rolodato
Copy link
Member

@rolodato rolodato commented Oct 16, 2024

The current beta version of this library doesn't work when used from ESM:

// test.mjs
import Flagsmith, { EnvironmentModel } from "./build/esm/index.js"
console.log(Flagsmith, EnvironmentModel)
% node index.mjs
file:///Users/rolodato/source/flagsmith/flagsmith-nodejs-client/test/node_modules/flagsmith-nodejs/build/esm/flagsmith-engine/identities/models.js:2
const { v4: uuidv4 } = require('uuid');
                       ^

ReferenceError: require is not defined in ES module scope, you can use import instead
    at file:///Users/rolodato/source/flagsmith/flagsmith-nodejs-client/test/node_modules/flagsmith-nodejs/build/esm/flagsmith-engine/identities/models.js:2:24
    at ModuleJob.run (node:internal/modules/esm/module_job:195:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:337:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:106:12)

Removing this use of require reveals another use of module.exports, which is CJS and does not get compiled:

% node test.mjs
file:///Users/rolodato/source/flagsmith/flagsmith-nodejs-client/build/esm/index.js:4
module.exports = Flagsmith;
^

ReferenceError: module is not defined in ES module scope
    at file:///Users/rolodato/source/flagsmith/flagsmith-nodejs-client/build/esm/index.js:4:1
    at ModuleJob.run (node:internal/modules/esm/module_job:195:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:337:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:106:12)

Fixing this makes everything work:

% node test.mjs 
[class Flagsmith] [class EnvironmentModel]

As a bonus, we can add Flagsmith as a named export, for consistency:

// test-default.mjs
import { Flagsmith, EnvironmentModel } from "./build/esm/index.js"
console.log(Flagsmith, EnvironmentModel)
% node test-default.mjs 
[class Flagsmith] [class EnvironmentModel]

Important: this is a breaking change for CJS users, as require now returns an object instead of the Flagsmith class:

> require('./build/cjs/index.js')
{
  AnalyticsProcessor: [Getter],
  FlagsmithAPIError: [Getter],
  FlagsmithClientError: [Getter],
  EnvironmentDataPollingManager: [Getter],
  DefaultFlag: [Getter],
[...]

This PR also has some QoL changes:

  • 6c1d1b9 removes esModuleInterop - compiled code doesn't change, but this makes the code truly ESM-only
  • 4cc7097, a9e9e6c replace deps with built-in Node functions - this makes the test suite take ~4 seconds vs ~120 previously
  • 8eb2703 upgrade GHA workflow - this makes the CI run take ~17s vs ~3m previously

@kyle-ssg kyle-ssg marked this pull request as draft October 17, 2024 13:48
@rolodato rolodato changed the title Remove all uses of CJS, add named Flagsmith export feat!: Remove all uses of CJS, add named Flagsmith export Oct 22, 2024
@rolodato rolodato marked this pull request as ready for review October 22, 2024 17:53
Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me. One main question about a possible breaking change that wasn't listed in the description. Once that's understood, I think we can move forward and get this released.

flagsmith-engine/segments/models.ts Show resolved Hide resolved
flagsmith-engine/utils/hashing/index.ts Show resolved Hide resolved
flagsmith-engine/utils/hashing/index.ts Show resolved Hide resolved
tests/engine/e2e/engine.test.ts Show resolved Hide resolved
@matthewelwell matthewelwell merged commit 2cc95c7 into main Nov 7, 2024
1 check passed
@rolodato
Copy link
Member Author

rolodato commented Nov 7, 2024

Bonus - hashing is now over 100x faster

Index: tests/engine/unit/utils/utils.bench.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tests/engine/unit/utils/utils.bench.ts b/tests/engine/unit/utils/utils.bench.ts
new file mode 100644
--- /dev/null	(date 1730987845619)
+++ b/tests/engine/unit/utils/utils.bench.ts	(date 1730987845619)
@@ -0,0 +1,17 @@
+import {bench} from "vitest";
+import {
+    getHashedPercentateForObjIds,
+    getHashedPercentateForObjIdsOld
+} from "../../../../flagsmith-engine/utils/hashing/index.js";
+import {randomUUID as uuidv4} from "crypto";
+
+bench('old hashing', () => {
+    const _ = getHashedPercentateForObjIdsOld([uuidv4()])
+}, {
+    signal: AbortSignal.timeout(10000)
+})
+bench('new hashing', () => {
+    const _ = getHashedPercentateForObjIds([uuidv4()])
+}, {
+    signal: AbortSignal.timeout(10000)
+})
% npx vitest bench
Benchmarking is an experimental feature.
Breaking changes might not follow SemVer, please pin Vitest's version when using it.

 DEV  v2.1.2 /Users/rolodato/source/flagsmith/flagsmith-nodejs-client

 ✓ tests/engine/unit/utils/utils.bench.ts (2) 1303ms
     name                 hz     min     max    mean     p75     p99    p995    p999     rme  samples
   · old hashing    4,372.66  0.1842  0.3767  0.2287  0.2331  0.3018  0.3185  0.3650  ±0.39%     2187
   · new hashing  578,653.03  0.0014  1.3175  0.0017  0.0016  0.0028  0.0030  0.0053  ±1.63%   289327   fastest


 BENCH  Summary

  new hashing - tests/engine/unit/utils/utils.bench.ts
    132.33x faster than old hashing

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 this pull request may close these issues.

2 participants