Update TypeDoc and docs #232

ryanio · 2020-01-31T00:27:08Z

This PR updates TypeDoc and docs, closes #230.

Thanks @holgerd77 for the updated script.

ryanio · 2020-01-31T00:28:21Z

README.md

@@ -1,7 +1,7 @@
 # SYNOPSIS

 [![NPM Package](https://img.shields.io/npm/v/ethereumjs-util.svg?style=flat-square)](https://www.npmjs.org/package/ethereumjs-util)
-[![Actions Status](https://github.com/ethereumjs/ethereumjs-util/workflows/Build/badge.svg)](https://github.com/ethereumjs/ethereumjs-util/actions
+[![Actions Status](https://github.com/ethereumjs/ethereumjs-util/workflows/Build/badge.svg)](https://github.com/ethereumjs/ethereumjs-util/actions)


also fixed a missing ) here.

github-actions · 2020-01-31T00:28:32Z

Coverage remained the same at 99.644% when pulling 98c1de0 on updateTypeDoc into 3f8dc23 on master.

ryanio · 2020-01-31T00:40:42Z

docs/README.md


-**● setLength**: *[setLengthLeft]()* =  setLengthLeft
+• **rlp**: *"/Users/ryanghods/dev/ethereumjs-util/node_modules/rlp/dist/index"*


Huh this is funny. It looks like it is trying to automatically document this line import rlp = require('rlp') and accidentally inserting my file system path. Didn't seem to happen anywhere else though. I'll see if there is any way to solve it.

Hi Ryan, thanks for having a start on this. Yeah, this is what I meant in the issue description comment in the last paragraph mentioning the require syntax used.

This would need some deeper analysis.

Some possible ideas:

Eventually it is possible to replace some of this special require syntax includes with normal import statements (would be best)

There might be a TypeDoc option I have missed to prevent this from being included in the docs (second best)

Eventually there is some for of comment syntax to be placed above the respective require statements to indicate to TypeDoc to not to include the lines (third best, no problem though either)

holgerd77 · 2020-01-31T09:40:34Z

docs/README.md

+* [KECCAK256_RLP_S](README.md#const-keccak256_rlp_s)
+* [MAX_INTEGER](README.md#const-max_integer)
+* [TWO_POW256](README.md#const-two_pow256)
+* [assert](README.md#const-assert)


This is the case I mentioned in the issue, having this assert here is completely nonsense.

holgerd77 · 2020-01-31T09:43:14Z

Also on a second thought along having a look into the PR: it would actually be a lot nicer to have the documentation also structured along the different modules like account, bytes, constants and so on. Maybe you can also have a look from this angle and include this in the PR as well.

If it gets too complicated (writing some doc template or whatever, but also depends on the complexity) it might not be worth it, but otherwise it would make the docs a lot more readable.

ryanio · 2020-01-31T21:21:50Z

@holgerd77 ok thanks I will investigate further, would be great to have it split by module like you suggested.

Do you have any thoughts on adding npm run docs:build as a husky precommit or gh action so docs are automatically regenerated after any changes?

* create `src/externals.ts` and `test/externals.spec.ts`

ryanio · 2020-02-03T19:23:43Z

I discovered a new library mode feature in typedoc@next that seems to align with what we're trying to accomplish here by separating the docs by src file.

I pushed a commit if you want to take a look and see what you think of the generated docs with the new configuration in typedoc.js.

I also added a src file externals.ts that handles the re-exports. I added some @packageDocumentation to the top of the file since typedoc still isn't doing a great job explaining what's going on, however it is additionally complex due to the use of Object.assign(exports, ethjsUtil) which already has its own issue #219.

Converting import rlp = require('rlp') syntax to import * as rlp from 'rlp' seems to have fixed the issue of those lines being documented.

Side note, husky wasn't letting me push without running npm run lint:fix first that did a lot of prettifying again, so it is annoying the diffs are all crowded. Let me know if there's a better workflow to get around this, and I'm wondering why these files weren't already up to spec.

holgerd77

Looks good, will merge, thanks Ryan!

holgerd77 · 2020-02-04T10:35:37Z

package.json

@@ -112,8 +113,8 @@
    "prettier": "^1.15.3",
    "ts-node": "^8.6.2",
    "tslint": "^5.12.0",
-    "typedoc": "^0.14.0",
-    "typedoc-plugin-markdown": "^1.1.21",
+    "typedoc": "next",


Great find with this library mode! Just browsed through the previews of the documentation files, that is exactly what we need here I would say. This makes the functionality of this library so much more approcheable, great! 😄

This next tag is pretty much unspecific and takes out some determinism for a dev installation. Think in this case it's worth though also considering that this is just a dev dependency for the doc generation, so it is not even changing "implicit" dev environment behavior (if there is a new next version) but will be clearly noted if doc generation behaves differently.

We just should remember to update this once there is the occasion and this gets officially published.

Yes it was relieving to find library mode, it perfectly fit our needs :) Agreed about moving from the next tag as soon as we can, I’ll keep an eye out for releases.

holgerd77 · 2020-02-04T10:36:14Z

package.json

@@ -100,6 +100,7 @@
    "@ethereumjs/config-tslint": "^1.1.0",
    "@types/mocha": "^5.2.7",
    "@types/node": "^11.9.0",
+    "@types/secp256k1": "3.5.0",


For the reference: new @types/secp2561 dependency

Yes, thanks, by the way this should be updated to 3.5.2 for the new types in the PR for updating secp256k1 to latest (although I’m sure it’ll split out an error letting you know after rebasing on master :)

Also maybe we can add a question “Does this PR add or modify any dependencies?” to a contributing PR template so we can always have that information in the OP, I forget sometimes to mention (sorry!)

We have one for Grid here that is nice to have, I’m sure we can write something similar for the monorepo. https://github.com/ethereum/grid/blob/master/.github/PULL_REQUEST_TEMPLATE.md

Yes, would likely make sense to have some unified PR template throughout the EthereumJS libraries. Would be something to be proposed and discussed here https://github.com/ethereumjs/organization if we want to continue on that.

holgerd77 · 2020-02-04T10:37:17Z

src/signature.ts

-const secp256k1 = require('secp256k1')
-import BN = require('bn.js')
+import * as secp256k1 from 'secp256k1'
+import * as BN from 'bn.js'
 import { toBuffer, setLength, setLengthLeft, bufferToHex } from './bytes'
 import { keccak } from './hash'



Did have a look at all module files, also checked the linking from README. This looks good and consistent now.

holgerd77 · 2020-02-04T10:37:50Z

src/hash.ts

@@ -1,7 +1,7 @@
 const createKeccakHash = require('keccak')
 const createHash = require('create-hash')
 const ethjsUtil = require('ethjs-util')
-import rlp = require('rlp')


Cool that this ugly syntax version could be replaced on so many occurrences.

holgerd77 · 2020-02-04T10:42:19Z

src/externals.ts

+/**
+ * [`secp256k1`](https://github.com/cryptocoinjs/secp256k1-node/)
+ */
+export { secp256k1 }


Nice to have this in a separate file now, this generally make library exports easier to grasp and more explicit, e.g. to have this new externals.spec.ts test file along is a great addition.

I first thought if it would be necessary to hide this respectively just have this for documentation purposes and recommend (e.g. by stating in the docs) to use the index.ts exposure for importing the re-exports as a user. Second thought lead me to think that this is not necessary and it is very much ok to have this new externals entrypoint as an alternative. This will likely stay pretty stable and remains fully backwards-compatible.

holgerd77 · 2020-02-04T10:52:34Z

Ah, the Husky questions:

Build docs on Husky precommit: we discussed some time ago WHEN to actually build the documentation with some people - think @alcuadrado and @s1na - and we came to the conclusion (sorry, don't find the reference any more) that is best for the end user of a library when the docs build meet the last release version and don't just correspond to master.

So we would want to establish the practice here to always rebuild the documentation along a release - this would likely be my job - this hasn't completely established though yet.

It's also likely to have some exceptions here, e.g. on a PR like this one probably doesn't want to wait with building the docs and keep this unproven to work until the next release.

Workflow for lint run on pushing: this is eventually if code hasn't been touched for some longer time and linting rules changed with new TSLint or Prettier releases? Not sure though. Also have no brilliant idea for a workflow improvement, this is likely getting better once we get on more active work on the different libraries (in progress... 😋).

evertonfraga · 2020-02-04T18:10:26Z

Here's the discussion where @s1na suggests to trigger a job in the event of a release.

In the following comments, I suggested that the CI job would build the docs and perform a diff. If it has any diff from HEAD, the job would fail and we should update it manually. It might seem more complicated, but IMO CI services shouldn't have push access to the repository.

ryanio · 2020-02-04T18:36:49Z

nice @evertonfraga thank you for the link and context. i like the git diff with exit code idea, very neat, and could be simple to trigger with gh actions on new tags and/or releases.

holgerd77 · 2020-02-05T11:01:07Z

@evertonfraga took me some time to grasp all your guys' ideas and think through the practical implications, but I am finally there. 🙃

So - yes - would think this combination of using the release event to trigger a GitHub action flow and then do the diff comparison (push access is really a good objection here) might really be a good combination to get this under control + keep some necessary level of freedom respectively flexibility here.

update typedoc and docs

98c1de0

ryanio commented Jan 31, 2020

View reviewed changes

holgerd77 reviewed Jan 31, 2020

View reviewed changes

* use library mode in typedoc@next

59e9849

* create `src/externals.ts` and `test/externals.spec.ts`

holgerd77 approved these changes Feb 4, 2020

View reviewed changes

holgerd77 merged commit ae4c300 into master Feb 4, 2020

holgerd77 deleted the updateTypeDoc branch February 4, 2020 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update TypeDoc and docs #232

Update TypeDoc and docs #232

ryanio commented Jan 31, 2020 •

edited

Loading

ryanio Jan 31, 2020 •

edited

Loading

github-actions bot commented Jan 31, 2020

ryanio Jan 31, 2020

holgerd77 Jan 31, 2020

holgerd77 Jan 31, 2020

holgerd77 commented Jan 31, 2020

ryanio commented Jan 31, 2020

ryanio commented Feb 3, 2020

holgerd77 left a comment

holgerd77 Feb 4, 2020

ryanio Feb 4, 2020

holgerd77 Feb 4, 2020

ryanio Feb 4, 2020

ryanio Feb 4, 2020

holgerd77 Feb 5, 2020

holgerd77 Feb 4, 2020

holgerd77 Feb 4, 2020

holgerd77 Feb 4, 2020

holgerd77 commented Feb 4, 2020

evertonfraga commented Feb 4, 2020

ryanio commented Feb 4, 2020

holgerd77 commented Feb 5, 2020


		● setLength: [setLengthLeft]() = setLengthLeft
		• rlp: "/Users/ryanghods/dev/ethereumjs-util/node_modules/rlp/dist/index"

Update TypeDoc and docs #232

Update TypeDoc and docs #232

Conversation

ryanio commented Jan 31, 2020 • edited Loading

ryanio Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Jan 31, 2020

ryanio commented Jan 31, 2020

ryanio commented Feb 3, 2020

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Feb 4, 2020

evertonfraga commented Feb 4, 2020

ryanio commented Feb 4, 2020

holgerd77 commented Feb 5, 2020

ryanio commented Jan 31, 2020 •

edited

Loading

ryanio Jan 31, 2020 •

edited

Loading