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

super small code polish #5276

Merged
merged 1 commit into from
Oct 20, 2022
Merged

super small code polish #5276

merged 1 commit into from
Oct 20, 2022

Conversation

shunjizhan
Copy link
Contributor

No description provided.

@jacogr
Copy link
Member

jacogr commented Oct 18, 2022

There is actually method behind this madness - with the ?. babel generates horrible code for environments aka "a complete overkill for method availability checks", so generally we try and stay away from the operator.

This is the actual compiled output after the change is made -

      var _api$rpc$system;

      if (!((_api$rpc$system = api.rpc.system) !== null && _api$rpc$system !== void 0 && _api$rpc$system.dryRun)) {
        throw new Error('The system.dryRun RPC call is not available in your environment');
      }

This applies to at least in the api and the utils, there has actually been a conscious effort to not use it - however it certainly have not been consistently removed all-over polkadot-js repos. If left to the engine in all cases, it would be less messy, but we are not quite at that point with JS support everywhere as of yet.

@wirednkod
Copy link
Member

Did NOT know about that.. Thank you @jacogr

@wirednkod wirednkod requested review from wirednkod and removed request for wirednkod October 18, 2022 10:49
@shunjizhan
Copy link
Contributor Author

shunjizhan commented Oct 18, 2022

There is also similar concern about for ... of since it transpiles to generators. I guess there is always a tradeoff between better performance and cleaner source code.

Personally I am more into cleaner code, because of two reasons:

  • we don't really need to read the prod es5 code, so its ok to be messy
  • the "performance downgrade" introduced by generator or more messy generated code is trivial for an app like this, where internet traffic takes much longer than code running (for frontend prod code this might be a small concern though)

@shunjizhan
Copy link
Contributor Author

But either way I think some eslint rule to enforce the style (force optional chaining/no optional chaining) would be great

@jacogr
Copy link
Member

jacogr commented Oct 18, 2022

There are no eslint (nor ts-eslint) rules to deny these. Also as mentioned, in UI-code, due to what you mentioned above, it has not been a "rule" and is certainly used a lot. (The API and util is very different from "UI code")

In general, these are not that bad in the PR form, where it becomes hairy and worth screaming is when doing a?.b?.c?.d. So each time I use one of these, my brain just screams at me since it know what it causes :)

However, it is actually fixable with a babel == null or != null output with assumptions, specifically noDocumentAll: true - that would remove any "damn, I'm writing really bad stuff now" concerns that causes minor brain explosions. (And would apply to all repos equally if added to the linked config).

With that in and non-hairy outputs, there are actually ts-eslint rules that can enforce the usage of these. (Which would certainly be ok in that case)

@shunjizhan
Copy link
Contributor Author

sounds great, so I am closing this PR as it's not the desired syntax

@shunjizhan shunjizhan closed this Oct 18, 2022
@jacogr
Copy link
Member

jacogr commented Oct 19, 2022

Feel free to re-open, adjusted the babel outputs slightly with https://github.com/polkadot-js/dev/pull/827/files#diff-e46f7bc448773ef1bc430c11187663a5431bc8d7a67aba417675b2e31f3e8cf1R6-R7

The above now certainly removes my issues with the outputs, especially on those pesky longer items, it is no no worse than doing it "manually".

@shunjizhan shunjizhan reopened this Oct 20, 2022
@jacogr jacogr merged commit 52ef278 into polkadot-js:master Oct 20, 2022
@polkadot-js-bot
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Oct 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants