feat(contract): linting and typedefs #26
Conversation
contract/package.json
Outdated
| "lint-fix": "eslint --fix '**/*.{js,ts}'", | ||
| "lint-fix-jessie": "eslint -c '.eslintrc-jessie.js' --fix '**/*.js'", | ||
| "lint-jessie": "eslint -c '.eslintrc-jessie.js' '**/*.js'", | ||
| "typecheck": "tsc" |
There was a problem hiding this comment.
not critical:
elsewhere this is called lint:types and we have lint:eslint and lint does both.
see the PR linked from Agoric/agoric-sdk#8274 (reply in thread)
There was a problem hiding this comment.
Agree, didn't want to change lint-* as a part of this, but agree it should standardized.
contract/package.json
Outdated
| "@endo/bundle-source": "^2.8.0", | ||
| "@endo/eslint-plugin": "^0.5.2", | ||
| "@endo/init": "^0.5.60", | ||
| "@endo/promise-kit": "^1.0.1", |
There was a problem hiding this comment.
important
That version smells like trouble. We need to keep the endo versions consistent, and I'm pretty sure this dapp is not up to the 1.x stuff yet.
cf
There was a problem hiding this comment.
Good catch. Updated to 0.2.56
| @@ -73,13 +76,47 @@ | |||
| }, | |||
| "homepage": "https://github.com/agoric-labs/dapp-join-game#readme", | |||
| "eslintConfig": { | |||
| "env": { | |||
There was a problem hiding this comment.
Whatever knowledge you have learned about lint config, would you please contribute it to...
There was a problem hiding this comment.
Sure, let me gather some feedback here first to make sure the approach is correct. The main changes are -
- use typescript as the eslint parser
- add
no-voidandno-floating-promisesrules - reimplement
_ignorepatterns for unused-vars, as@typescript-eslintalso checks for this (curious - can we dedupeno-unused-varsand@typescript-eslint/no-usused-vars. Both are effectively doing the same thing)?
Here would be the updated directions from the current answer:
- Run:
yarn add -D \
eslint \
@agoric/eslint-config@dev \
@endo/eslint-plugin \
@jessie.js/eslint-plugin \
eslint-config-airbnb-base \
eslint-plugin-jsdoc \
eslint-config-prettier \
eslint-plugin-import \
- eslint-plugin-github
+ eslint-plugin-github \
+ @typescript-eslint/eslint-plugin
+ @typescript-eslint/parser
+ typescript- Add the following to your
package.json
"eslintConfig" : {
+ "env": {
+ "node": true
+ },
+ "parser": "@typescript-eslint/parser",
"parserOptions": {
- "sourceType": "module",
- "ecmaVersion": 6
+ "project": "./tsconfig.json",
+ "sourceType": "module",
+ "ecmaVersion": 2020
},
"extends": [
+ "plugin:@typescript-eslint/recommended",
"@agoric"
],
+ "plugins": [
+ "@typescript-eslint",
+ "prettier"
+ ],
+ "rules": {
+ "@typescript-eslint/no-floating-promises": "warn",
+ "no-void": [
+ "error",
+ {
+ "allowAsStatement": true
+ }
+ ],
+ "prettier/prettier": "warn",
+ "@typescript-eslint/no-unused-vars": [
+ "error",
+ {
+ "vars": "all",
+ "args": "all",
+ "argsIgnorePattern": "^_",
+ "varsIgnorePattern": "^_"
+ }
+ ]
+ },
}
- Add a tsconfig.json file (new)
+{
+ "compilerOptions": {
+ "noEmit": true,
+ "target": "esnext",
+ "module": "esnext",
+ "moduleResolution": "node",
+ "skipLibCheck": true,
+ "checkJs": false,
+ "allowJs": true,
+ "allowSyntheticDefaultImports": true,
+ "maxNodeModuleJsDepth": 2,
+ "strict": true
+ },
+ "include": ["**/*.js", "**/*.ts"],
+ "exclude": ["bundles"]
+}- Run
yarn eslint '**/*.{js,ts}'(or specify other files/directory globs). Runyarn tscto lint types.
contract/package.json
Outdated
| }, | ||
| "prettier": { | ||
| "trailingComma": "all", | ||
| "arrowParens": "avoid", | ||
| "singleQuote": true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Prettier mishap, should be fixed now
contract/src/gameAssetContract.js
Outdated
| @@ -23,8 +23,9 @@ const bagValueSize = amt => { | |||
| return total; | |||
| }; | |||
|
|
|||
| /** @typedef {StandardTerms & { joinPrice: Amount<'nat'> }} GamePlacesTerms */ | |||
There was a problem hiding this comment.
important: Why constrain it to nat amounts?
StandardTerms is redundant here. The parameter to the ZCF<...> type is custom terms.
There was a problem hiding this comment.
StandardTerms is redundant here. The parameter to the ZCF<...> type is custom terms.
Agree/ack. This was added in an attempt to get const terms = await E(zoe).getTerms(instance); inside test-contract.js to not complain Property 'brands' does not exist on type 'GamePlacesTerms'..
Open to hints for getting StandardTerms recognized properly in the testing context.
contract/test/test-bundle-source.js
Outdated
| import { E, passStyleOf } from '@endo/far'; | ||
| import { E } from '@endo/eventual-send'; | ||
| import { passStyleOf } from '@endo/far'; |
There was a problem hiding this comment.
This was added since anytime E is imported from @endo/far, instead of @endo/eventual-send, typescript will complain that This expression is not callable. Type 'never' has no call signatures
There was a problem hiding this comment.
Sounds like a bug in @endo/far's type exports. Hopefully fixed in subsequent releases.
Either way, eventual-send is the source: https://github.com/endojs/endo/blob/2179108cb9247e9499c1f319de907d7cd365f314/packages/far/src/index.js#L1
@endo/far is just for convience. https://github.com/endojs/endo/blob/master/packages/far/README.md#L6
There was a problem hiding this comment.
as noted above, @endo/far is the norm / convention. It started a couple years ago. I'm not in a position to approve a migration away from it on my own.
There was a problem hiding this comment.
Tracking this here #31 to keep this PR moving
| @@ -187,6 +189,14 @@ test('use the code that will go on chain to start the contract', async t => { | |||
| }); | |||
|
|
|||
| const { zoe } = t.context; | |||
| /** | |||
| * @param {{ | |||
| * installation: ERef<Installation<GameContractFn>>; | |||
There was a problem hiding this comment.
GameContractFn seems overly specific here. But perhaps the relevant generic type is a pain.
yarn.lock
Outdated
| @@ -6168,6 +6185,13 @@ ses@^0.18.4, ses@^0.18.5, ses@^0.18.8: | |||
| dependencies: | |||
| "@endo/env-options" "^0.1.4" | |||
|
|
|||
| ses@^1.0.1: | |||
There was a problem hiding this comment.
more problematic endo/ses versions
| @@ -38,28 +44,25 @@ const publishBrandInfo = async (chainStorage, board, brand) => { | |||
|
|
|||
| /** | |||
| * Core eval script to start contract | |||
| * | |||
| * @param {BootstrapPowers} permittedPowers | |||
| * XXX FIXME File '~/agoric-sdk/packages/vats/src/core/types.js' is not a module | |||
There was a problem hiding this comment.
importing the ambient for side-effects should work.
https://github.com/Agoric/agoric-sdk/blob/master/packages/deploy-script-support/test/unitTests/test-coreProposalBehavior.js#L11-L14
- _agstate/yarn-links/**/* will be present if consumers use 'agoric install'
- contract is not dependent on @babel/code-frame or @babel/highlight, only @vitejs/react-plugin is
- uses dependencies from @agoric/zoe@0.26.3-u13.0 https://github.com/Agoric/agoric-sdk/blob/5a6cdeb0c18ae9700d706445acf402f8d1e873c3/packages/zoe/package.json
- '&& yarn lint:types' is commented until lint:types passes
- add script to bundle contract and build proposal. currently, build only happens in docker
contract/package.json
Outdated
| "build": "exit 0", | ||
| "build": "node_modules/agoric/bin/agoric run scripts/build-game1-start.js", |
There was a problem hiding this comment.
is node_modules/agoric/bin/agoric necessary? Doesn't yarn arrange for agoric to work?
There was a problem hiding this comment.
The scope of this PR seems to be expanding :)
Do you plan to keep the commits separate or squash?
| "lint:fix": "eslint --fix '**/*.{js,ts}'", | ||
| "lint:types": "tsc" | ||
| }, | ||
| "dependencies": { |
There was a problem hiding this comment.
moving the "dependencies" section makes this diff hard to read.
| "@agoric/ertp": "0.16.3-u13.0", | ||
| "@agoric/zoe": "0.26.3-u13.0", |
There was a problem hiding this comment.
Have you checked that the resulting bundle is a reasonable size? that we have just 1 version of each endo package?
| @@ -470,6 +509,19 @@ | |||
| "@endo/nat" "4.1.27" | |||
| better-sqlite3 "^8.2.0" | |||
|
|
|||
| "@agoric/swing-store@^0.9.2-u13.0": | |||
There was a problem hiding this comment.
why do we have whole new packages here?
ensures
yarn workspace game-places-contract lintpasses 4a7fdbd, a6496a0, 7e3d0edupdates dependencies to
agoric-upgrade-13versionsensuresyarn workspace game-places-contract typecheckyields no errors f2d6dfcsee
yarn workspace game-places-contract lint:typesfails #32 and E (Eventual Send) type 'never' has no call signature #31Testing
Tested generated bundle sizes with both
yarn installandagoric install. I also tested the case described in #28: