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

Tiny fixes #1177

Merged
merged 15 commits into from
Apr 6, 2021
Merged

Tiny fixes #1177

merged 15 commits into from
Apr 6, 2021

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Mar 30, 2021

This PR combines some tiny fixes from #905, including:

This PR additionally:

  • Updates typedoc to latest, removes some deprecated options
  • Fixes devp2p peer communication examples with new tx library updates

I haven't regenerated the docs yet since it looks like it will change a few hundred files (since the library mode was removed) and I wanted to make the review easier, so I can add that as a last commit after review, or before the next release, whichever you'd prefer @holgerd77.

@codecov
Copy link

codecov bot commented Mar 30, 2021

Codecov Report

Merging #1177 (f97dae7) into master (8733939) will decrease coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 82.25% <ø> (-0.06%) ⬇️
blockchain 84.23% <ø> (ø)
client 84.05% <ø> (+0.01%) ⬆️
common 87.39% <ø> (+0.52%) ⬆️
devp2p 83.88% <ø> (-0.42%) ⬇️
ethash 82.08% <ø> (ø)
tx 84.30% <ø> (ø)
vm 83.63% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@ryanio
Copy link
Contributor Author

ryanio commented Mar 30, 2021

client karma run is having some trouble with a downstream update: Uncaught TypeError: TextDecoder is not a constructor

it looks like it's from web-encoding v1.1.2 which was just released, i'm looking into if it's an issue on our end or theirs.

@holgerd77
Copy link
Member

@ryanio Cool! 😄 👍

Maybe don't regenerate the docs, I'd rather do this along the PR on the releases, so if we merge earlier here than I would do this on the release PR, then we are more flexible here and I would really like to get these releases out this week. If I will do the releases before the merge here it is also better to wait since it otherwise would get a bit tricky on rebase (I've also changed a lot of docs there).

@ryanio
Copy link
Contributor Author

ryanio commented Apr 1, 2021

ok this should be ready for review now! feel free to merge if it looks good.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks, nice clean-up! 😄 👍

@@ -11,7 +10,6 @@ module.exports = {
'lib/evm/**',
'lib/state/cache.ts',
],
excludeNotExported: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have got a clear reason to have this removed? I haven't got the complete oversight of the consequences right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes this option was removed:

This is mentioned in the release notes here: https://github.com/TypeStrong/typedoc/releases/tag/v0.20.0

Removed options - mode, excludeNotExported, includeDeclarations, ignoreCompilerErrors, entryPoint

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the real impacting change is that the experimental library mode was removed and there is a new default mode. so our newly regenerated docs after these changes may look quite different in structure and markup, but should hopefully be an improvement and align with the future development of the package.

@@ -58,10 +58,10 @@
"eslint-config-prettier": "^6.11.0",
"eslint-plugin-implicit-dependencies": "^1.0.4",
"istanbul": "^0.4.1",
"karma": "^5.2.1",
"karma-browserify": "^7.0.0",
"karma": "^6.3.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of interest: have you looked if this has any practical consequences (performance, test quality or realism 😄 ,...)?

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 was updating the deps in trying to resolve some miscellaneous issues but it was all working fine so i decided to keep the version bumps :)

@@ -15,7 +15,7 @@
"scripts": {
"postinstall": "npm run bootstrap",
"bootstrap": "lerna bootstrap --ignore-scripts --include-dependencies --no-ci --hoist && npm run temporaryClientFixes && npm run build",
"temporaryClientFixes": "cp node_modules/interface-datastore/dist/src/key.d.ts node_modules/interface-datastore/src && cd node_modules/bcrypto && npm i",
"temporaryClientFixes": "rm -r node_modules/acorn && cp -r node_modules/karma-typescript/node_modules/acorn/ node_modules/acorn/ && cp node_modules/interface-datastore/dist/src/key.d.ts node_modules/interface-datastore/src && cd node_modules/bcrypto && npm i",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give a (very short) description what is happening here (or have I missed?)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I should have noted this, thanks.

this is a small temporary workaround fix for an updated acorn version which karma-typescript needs for the es6-transform plugin (which i described here: #1177 (comment)). karma was hanging on the transform step when using an older version of acorn. our node_modules has several acorn version but the one on the top level that was being used was outdated, so i copy the newer version located inside karma-typescript/node_modules to the top level node_modules which resolved the problem.

i'll check back in a few weeks and if the issue is resolved (most likely by other modules updating their acorn dependency version) i can remove this temporary workaround 👍

@@ -102,6 +102,7 @@
"karma-firefox-launcher": "^2.1.0",
"karma-tap": "^4.2.0",
"karma-typescript": "^5.5.1",
"karma-typescript-es6-transform": "^5.5.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this introduction related to the quoted problem?

client karma run is having some trouble with a downstream update: Uncaught TypeError: TextDecoder is not a constructor

it looks like it's from web-encoding v1.1.2 which was just released, i'm looking into if it's an issue on our end or theirs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, so after I reported my bug they did a new release updating the browser file but it was an esmodules (es6) file so I needed this plugin to transform it to es5 to work in our pipeline.

@holgerd77 holgerd77 merged commit a141b52 into master Apr 6, 2021
@holgerd77 holgerd77 deleted the tiny-fixes branch April 6, 2021 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants