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

Conversation

yusinto
Copy link
Contributor

@yusinto yusinto commented Sep 21, 2023

Bundle cloudflare sdk using rollup and in process remove js-core and package.json imports.

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #217604: Rollup cloudflare sdk.

Comment on lines 20 to 25
"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",
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 moved typings one level up so we have 1 copy to share now between cjs and esm.

Comment on lines 20 to 22
"types": "./dist/index.d.ts",
"import": "./dist/esm/src/index.js",
"require": "./dist/cjs/src/index.js"
Copy link
Contributor Author

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",
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.

"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.

},
"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.

packages/sdk/cloudflare/src/index.ts Outdated Show resolved Hide resolved
packages/sdk/cloudflare/rollup.config.ts Outdated Show resolved Hide resolved
Comment on lines +79 to +81
"bundledDependencies": [
"@launchdarkly/js-server-sdk-common-edge"
]
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.

"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 

@@ -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]' };
Copy link
Member

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.

Copy link
Contributor Author

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.
@yusinto yusinto merged commit 7af4f6e into main Sep 21, 2023
13 checks passed
@yusinto yusinto deleted the yus/sc-217604/rollup-cloudflare-sdk branch September 21, 2023 20:09
@github-actions github-actions bot mentioned this pull request Sep 21, 2023
yusinto pushed a commit that referenced this pull request Sep 21, 2023
🤖 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>
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.

3 participants