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

Monorepo: Mark all _* named methods as protected (some less-strict flavor of private) #2852

Closed
6 tasks
holgerd77 opened this issue Jul 3, 2023 · 4 comments
Closed
6 tasks

Comments

@holgerd77
Copy link
Member

holgerd77 commented Jul 3, 2023

Sorry everyone that I am even coming up with this one 😂, but the UX/DX benefits are just too dramatic to not do it in this round.

Current code completion for - basically - our whole library items/classes look like the following, here for the EVM:

grafik

So bascially EVERTHING is displayed but not the official API. 😆

We should urgently update here, this would so drastically improve the whole experience and make API usage so much better.

Some pre-comments here:

  1. We have in the very most cases _* names for methods or properties which should not be accessed. Quickly thought if we would want to switch to "normal" naming here, but then I remembered that this distinction is actually good for the JavaScript users where these keywords like protected doesn't exist. So we should keep this as the standard.

  2. We should switch to protected (for the most part) and not to private. This also "does the job" and this helps if people want to extend a class or something, so it is just a bit more on the safe side of things.

  3. We should nevertheless take this task a bit loose. This will be a lot to do, if we get from 30% to 90% that would be already a huge achievement.

So what we should do is:

  •  Go through the libraries and mark all _* methods as protected, sometimes private might also be a good choice. This should be accompanied by a lot of test runs, since one can actually break several test setups with this.
  • If there are only 1-2, maybe 3 accesses of some side variable in the tests, we should rather go with any casting or something (or is there a better modifier with some "access nevertheless" for such a case which preserves the typing? 🤔)
  •  For more frequently accessed methods/properties (I guess this counts more for the properties part) one can also ask the other way around: does it make sense to expose this within the public API? (this can also be a frequent discussion point during the next days, if someone takes on the task)
  • I guess a special case is _common access from other libraries, which is a very common pattern within the monorepo and we might want to discuss separately. Do we want the Common objects to be generally exposed within the API?
  • Also, careful other way around as well: are there non _* methods/properties which are in fact rather protected/private? These should then get an underscore! I think DEBUG is a prominent example.
  • Special (easier) case here: the above and already marked as protected. So this should be checked by a package or monorepo search: are all methods/properties marked as protected written with _?

Not sure if I am missing some aspects here, then just drop some notes on this below or discuss in the chat!

@holgerd77
Copy link
Member Author

And, as some update, and just to be mentally prepared: this is a task not to be underestimated, a bit in the range of the prefixed/unprefixed hex string work Jochem just did and which ended up to be a huge PR.

So if you take count in several days rather than hours. 🙂

@holgerd77
Copy link
Member Author

Also dropping here as additional note/help:

Just stumbled upon this "intentional escape hatch" "trick" with TypeScript which allows you access private/protected class members without any typing and so without loosing the typing. 🤯

// intentional escape hatch
console.log(test['bar'])

Just tested this with console.log(evm['_common'].hardfork()) and can confirm that this works (also see screenshot).

Guess we can directly use this for the above issue (and caveat: for access only, write still doesn't work).

@jochem-brouwer
Copy link
Member

I think for at least EVM this is going to be a laborious task, since we access a lot of these underscore properties there. (We should not do this, and we should rewrite/refactor). But: yes, this is definitely not an easy/quick/trivial task.

@holgerd77
Copy link
Member Author

Closed by #2857

@gabrocheleau gabrocheleau removed their assignment Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants