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: rollup cloudflare sdk #279

Merged
merged 14 commits into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ module.exports = {
'import/no-extraneous-dependencies': [
'error',
{
devDependencies: ['**/jest*.ts', '**/*.test.ts'],
devDependencies: ['**/jest*.ts', '**/*.test.ts', '**/rollup.config.ts'],
},
],
},
Expand Down
4 changes: 2 additions & 2 deletions packages/sdk/cloudflare/example/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"module": "./dist/index.mjs",
"packageManager": "[email protected]",
"dependencies": {
"@launchdarkly/cloudflare-server-sdk": "^2.0.2"
"@launchdarkly/cloudflare-server-sdk": "^2.1.4"
},
"devDependencies": {
"@cloudflare/workers-types": "^4.20230321.0",
Expand All @@ -23,6 +23,6 @@
"build": "node build.js",
"start": "wrangler dev",
"deploy": "wrangler publish",
"test": "yarn build && node --experimental-vm-modules --no-warnings node_modules/jest/bin/jest.js"
"test": "yarn build && jest"
}
}
25 changes: 16 additions & 9 deletions packages/sdk/cloudflare/example/src/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,22 @@
import app from './index';
import testData from './testData.json';

test('variation true', async () => {
// arrange
const env = getMiniflareBindings();
const { LD_KV } = env;
await LD_KV.put('LD-Env-test-sdk-key', JSON.stringify(testData));
describe('test', () => {
let env: Bindings;

// act
const res = await app.fetch(new Request('http://localhost'), env);
beforeEach(async () => {
env = getMiniflareBindings();
const { LD_KV } = env;
await LD_KV.put('LD-Env-test-sdk-key', JSON.stringify(testData));
});

// assert
expect(await res.text()).toContain('testFlag1: true');
test('variation true', async () => {
const res = await app.fetch(new Request('http://localhost/?email=truemail'), env);
expect(await res.text()).toContain('testFlag1: true');
});

test('variation false', async () => {
const res = await app.fetch(new Request('http://localhost/?email=falsemail'), env);
expect(await res.text()).toContain('testFlag1: false');
});
});
6 changes: 5 additions & 1 deletion packages/sdk/cloudflare/example/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ export default {
async fetch(request: Request, env: Bindings): Promise<Response> {
const sdkKey = 'test-sdk-key';
const flagKey = 'testFlag1';
const context = { kind: 'user', key: 'test-user-key-1', email: '[email protected]' };
const { searchParams } = new URL(request.url);

// falsemail will return false, other emails return true
const email = searchParams.get('email') ?? '[email protected]';
const context = { kind: 'user', key: 'test-user-key-1', email };

// start using ld
const client = initLD(sdkKey, env.LD_KV);
Expand Down
2 changes: 1 addition & 1 deletion packages/sdk/cloudflare/example/src/testData.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"contextKind": "user",
"attribute": "/email",
"op": "contains",
"values": ["gmail"],
"values": ["falsemail"],
"negate": false
}
],
Expand Down
35 changes: 26 additions & 9 deletions packages/sdk/cloudflare/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,34 +17,43 @@
],
"type": "module",
"exports": {
"require": "./dist/cjs/src/index.js",
"import": "./dist/esm/src/index.js"
"types": "./dist/index.d.ts",
"import": "./dist/esm/index.js",
"require": "./dist/cjs/index.js"
},
"main": "./dist/cjs/src/index.js",
"types": "./dist/cjs/src/index.d.ts",
"main": "./dist/cjs/index.js",
"types": "./dist/index.d.ts",
"files": [
"dist"
],
"scripts": {
"build": "../../../scripts/build-package.sh",
Copy link
Contributor Author

@yusinto yusinto Sep 21, 2023

Choose a reason for hiding this comment

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

We no longer need to create sub package.jsons because it is now bundled with the sdk.

"clean": "rimraf dist",
"rb": "rollup -c --configPlugin typescript",
"rbw": "yarn rb --watch",
"build": "yarn clean && yarn rb",
"tsw": "yarn tsc --watch",
"start": "rimraf dist && yarn tsw",
"lint": "eslint . --ext .ts",
"prettier": "prettier --write '**/*.@(js|ts|tsx|json|css)' --ignore-path ../../../.prettierignore",
"test": "NODE_OPTIONS=\"--experimental-vm-modules --no-warnings\" jest --ci --runInBand",
"coverage": "yarn test --coverage",
"check": "yarn prettier && yarn lint && yarn build && yarn test && yarn doc"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yarn doc never existed so this command was failing before.

"check": "yarn prettier && yarn lint && yarn build && yarn test"
},
"dependencies": {
"@cloudflare/workers-types": "^4.20230321.0",
"@launchdarkly/js-server-sdk-common-edge": "1.0.13",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer a runtime dependency because all js-core deps are included in the bundle.

"crypto-js": "^4.1.1"
},
"devDependencies": {
"@launchdarkly/js-server-sdk-common-edge": "1.0.13",
"@rollup/plugin-commonjs": "^25.0.4",
"@rollup/plugin-json": "^6.0.0",
"@rollup/plugin-node-resolve": "^15.2.1",
"@rollup/plugin-terser": "^0.4.3",
"@rollup/plugin-typescript": "^11.1.3",
"@trivago/prettier-plugin-sort-imports": "^4.1.1",
"@types/crypto-js": "^4.1.1",
"@types/jest": "^29.5.0",
"@types/rollup-plugin-generate-package-json": "^3.2.3",
"@typescript-eslint/eslint-plugin": "^6.1.0",
"@typescript-eslint/parser": "^6.1.0",
"eslint": "^8.45.0",
Expand All @@ -57,9 +66,17 @@
"launchdarkly-js-test-helpers": "^2.2.0",
"miniflare": "^2.13.0",
"prettier": "^3.0.0",
"rimraf": "^5.0.0",
"rimraf": "^5.0.1",
"rollup": "^3.29.2",
"rollup-plugin-dts": "^6.0.2",
"rollup-plugin-esbuild": "^5.0.0",
"rollup-plugin-filesize": "^10.0.0",
"rollup-plugin-generate-package-json": "^3.2.0",
"ts-jest": "^29.1.0",
"typedoc": "0.25.0",
"typescript": "5.1.6"
}
},
"bundledDependencies": [
Copy link
Member

Choose a reason for hiding this comment

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

Won't this cause an unused copy of this to be bundled in the tgz?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, but I'll do a dry run and test.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like no with yarn pack. Maybe something we will want to check after publish.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we're safe because npm publish --dry-run says the tgz contents are:

npm notice === Tarball Contents === 
npm notice 556B    LICENSE              
npm notice 4.8kB   README.md            
npm notice 114.8kB dist/cjs/index.js    
npm notice 475.6kB dist/cjs/index.js.map
npm notice 114.8kB dist/esm/index.js    
npm notice 475.6kB dist/esm/index.js.map
npm notice 1.6kB   dist/index.d.ts      
npm notice 2.7kB   package.json 

"@launchdarkly/js-server-sdk-common-edge"
]
Comment on lines +79 to +81
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix eslint extraneous dep error by listing bundled js-core deps here.

}
53 changes: 53 additions & 0 deletions packages/sdk/cloudflare/rollup.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import commonjs from '@rollup/plugin-commonjs';
import json from '@rollup/plugin-json';
import resolve from '@rollup/plugin-node-resolve';
import terser from '@rollup/plugin-terser';
import dts from 'rollup-plugin-dts';
import esbuild from 'rollup-plugin-esbuild';
import filesize from 'rollup-plugin-filesize';

const inputPath = 'src/index.ts';
const cjsPath = 'dist/cjs/index.js';
const esmPath = 'dist/esm/index.js';
const typingsPath = 'dist/index.d.ts';

const plugins = [resolve(), commonjs(), esbuild(), json(), terser(), filesize()];

// the second array item is a function to include all js-core packages in the bundle so they
// are not imported or required as separate npm packages
const external = [/node_modules/, (id: string) => !id.includes('js-core')];

export default [
{
input: inputPath,
output: [
{
file: cjsPath,
format: 'cjs',
sourcemap: true,
},
],
plugins,
external,
},
{
input: inputPath,
output: [
{
file: esmPath,
format: 'esm',
sourcemap: true,
},
],
plugins,
external,
},
{
input: inputPath,
plugins: [dts(), json()],
output: {
file: typingsPath,
format: 'esm',
},
},
];
2 changes: 1 addition & 1 deletion packages/sdk/cloudflare/src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('init', () => {
});

test('rule match', async () => {
const contextWithEmail = { ...context, email: 'test@gmail.com' };
const contextWithEmail = { ...context, email: 'test@falsemail.com' };
const value = await ldClient.variation(flagKey1, contextWithEmail, false);
const detail = await ldClient.variationDetail(flagKey1, contextWithEmail, false);

Expand Down
2 changes: 1 addition & 1 deletion packages/sdk/cloudflare/src/testData.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"contextKind": "user",
"attribute": "/email",
"op": "contains",
"values": ["gmail"],
"values": ["falsemail"],
"negate": false
}
],
Expand Down
4 changes: 2 additions & 2 deletions packages/sdk/cloudflare/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
// Uses "." so it can load package.json.
"rootDir": ".",
"outDir": "dist",
"target": "es2017",
"target": "ES2017",
"lib": ["es6"],
"module": "commonjs",
"module": "ES6",
"strict": true,
"noImplicitOverride": true,
"allowSyntheticDefaultImports": true,
Expand Down
3 changes: 2 additions & 1 deletion scripts/build-package.sh
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#!/usr/bin/env bash
# Run this script like:
# ./scripts/build-package.sh

Expand All @@ -23,7 +24,7 @@ ESM_PACKAGE_JSON=$( jq -n \
--arg type "module" \
'{ name: $name, version: $version, type: $type }' )

tsc --module commonjs --outDir dist/cjs/
tsc --module commonjs --outDir dist/cjs/
echo "$CJS_PACKAGE_JSON" > dist/cjs/package.json

tsc --module es2022 --outDir dist/esm/
Expand Down