-
Notifications
You must be signed in to change notification settings - Fork 102
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(permit): refactor permit-utils package #3258
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Updated dependencies detected. Learn more about Socket for GitHub ↗︎
|
"name": "@cowprotocol/permit-utils", | ||
"version": "0.0.1-RC.0", |
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.
These fields define the final npm package.
"publish": { | ||
"command": "node tools/scripts/publish.mjs permit-utils {args.tag} {args.otp}", | ||
"dependsOn": ["build"] |
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.
Command used to publish:
npx nx run permit-utils:publish [--tag <tag>] [--otp <otp>]
Right now published to my personal account with a different package name https://www.npmjs.com/package/alfetopito-permit-utils?activeTab=code
libs/permit-utils/src/index.ts
Outdated
export { checkIsCallDataAValidPermit } from './lib/checkIsCallDataAValidPermit' | ||
export { generatePermitHook } from './lib/generatePermitHook' | ||
export { getPermitUtilsInstance } from './lib/getPermitUtilsInstance' | ||
export { getTokenPermitInfo } from './lib/getTokenPermitInfo' | ||
|
||
export type { PermitHookData, PermitHookParams, PermitInfo, PermitType, SupportedPermitInfo } from './types' |
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.
Lib interface
@@ -1,11 +1,9 @@ | |||
import { GP_VAULT_RELAYER } from '@cowprotocol/common-const' |
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.
Remove all internal cross dependencies
// Fetch the version from "package.json" before publishing | ||
let version | ||
try { | ||
const json = JSON.parse(readFileSync(`package.json`).toString()) | ||
version = json.version | ||
} catch (e) { | ||
console.error(chalk.bold.red(`Error reading package.json file from library build output.`)) | ||
process.exit(1) | ||
} |
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.
Changed the default publish command to use the package's version rathern than updating it based on cli param
tools/scripts/publish.mjs
Outdated
|
||
// Execute "npm publish" to publish | ||
const publishCommand = `npm publish --access public --tag ${tag === 'undefined' ? 'next' : tag}` | ||
const publishCommand = `npm publish --access public --tag ${tag === 'undefined' ? 'next' : tag} ${otp? `--otp ${otp}`:''}` |
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.
Optional opt
param, as I needed to publish it locally under my account.
Shouldn't be needed when moving this over to CI 🤞
26955ed
to
603a2d2
Compare
326664b
to
23d60f6
Compare
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.
Haven't tested, but code looks good!
Generated using `nx g @nx/js:lib` command
… generated bundle
aca2086
to
d1b55a2
Compare
11bc372
to
b03b2f8
Compare
"dependencies": { | ||
"ethers": "^5.7.2", | ||
"@1inch/permit-signed-approvals-utils": "^1.4.10", | ||
"@cowprotocol/app-data": "^1.1.0", | ||
"tslib": "^2.6.1" | ||
}, |
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.
Had to duplicate the dependencies here so package consumers install them too
"module": "./index.js", | ||
"main": "./index.cjs", | ||
"exports": { | ||
".": { | ||
"import": "./index.js", | ||
"require": "./index.cjs" | ||
} | ||
} | ||
} |
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.
This is so package can be consumed either the ES way (index.js) or CommonJS way (index.cjs)
}, | ||
rollupOptions: { | ||
// External packages that should not be bundled into your library. | ||
external: [/@1inch/, /@cowprotocol/, /@ethersproject/], |
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.
Explicitly removing third party dependencies to not have them included in the bundle.
They are instead added as part of package.json dependencies.
Summary
Refactoring permit fns into an isolated lib.
Removed all internal dependencies to have the lib isolated:
This lib is also to be published to npm in my personal account for now https://www.npmjs.com/package/alfetopito-permit-utils?activeTab=code
TODO:
To Test