-
Notifications
You must be signed in to change notification settings - Fork 18
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
Conversation
…f bundle. removed redundant typescript plugin.
…us yarn doc command.
This pull request has been linked to Shortcut Story #217604: Rollup cloudflare sdk. |
packages/sdk/cloudflare/package.json
Outdated
"types": "./dist/index.d.ts", | ||
"import": "./dist/esm/src/index.js", | ||
"require": "./dist/cjs/src/index.js" | ||
}, | ||
"main": "./dist/cjs/src/index.js", | ||
"types": "./dist/cjs/src/index.d.ts", | ||
"types": "./dist/index.d.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved typings one level up so we have 1 copy to share now between cjs and esm.
packages/sdk/cloudflare/package.json
Outdated
"types": "./dist/index.d.ts", | ||
"import": "./dist/esm/src/index.js", | ||
"require": "./dist/cjs/src/index.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
types
was missing and this caused an issue in the react sdk with typescript so this is a pre-emptive fix.
"files": [ | ||
"dist" | ||
], | ||
"scripts": { | ||
"build": "../../../scripts/build-package.sh", |
There was a problem hiding this comment.
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.
"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" |
There was a problem hiding this comment.
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.
}, | ||
"dependencies": { | ||
"@cloudflare/workers-types": "^4.20230321.0", | ||
"@launchdarkly/js-server-sdk-common-edge": "1.0.13", |
There was a problem hiding this comment.
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.
"bundledDependencies": [ | ||
"@launchdarkly/js-server-sdk-common-edge" | ||
] |
There was a problem hiding this comment.
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.
"ts-jest": "^29.1.0", | ||
"typedoc": "0.25.0", | ||
"typescript": "5.1.6" | ||
} | ||
}, | ||
"bundledDependencies": [ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@@ -4,7 +4,8 @@ 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]' }; | |||
// gmail will return false, other emails return true | |||
const context = { kind: 'user', key: 'test-user-key-1', email: '[email protected]' }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to use email addresses that did not involve trademarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
… example app. improve example test structure.
🤖 I have created a release *beep* *boop* --- <details><summary>cloudflare-server-sdk: 2.2.0</summary> ## [2.2.0](cloudflare-server-sdk-v2.1.4...cloudflare-server-sdk-v2.2.0) (2023-09-21) ### Features * rollup cloudflare sdk ([#279](#279)) ([7af4f6e](7af4f6e)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Bundle cloudflare sdk using rollup and in process remove js-core and package.json imports.