Revert non-essential changes (#3190)

[cgewecke]

Description

PR targets #3190

@nivida, I doubt you will be fond of this 🙂 Apologies in advance.

Continuing in the spirit of #3303 and getting #3190 into compliance with the PR Guidelines, this PR drops ~40 files from #3190

Removed are:

• package-lock changes for modules without dep changes
• non-essential TS re-org. (Have preserved the key changes in core-helpers & ws-provider)
• all build artifacts
• ENS module changes. (as out of scope)

All tests pass and the change-set is reduced from 63 to 24 files. The new scope can be seen most clearly by re-targeting this PR at 1.x.

Am not questioning the value of the things I'm proposing to remove here - just trying to reduce #3190's complexity and make its diff more legible.

#3190 rewrites some of the most sensitive and least-tested parts of the code, introducing excellent fixes. This PR just tries to tighten its focus to what's essential.

Type of change

• Review suggestion

[coveralls]

Coverage increased (+0.02%) to 88.063% when pulling a84df3f on pri/minimize-changeset into cd680eb on provider-related-improvements.

[nivida]

Thanks for going through. We have to discuss the following points,

• Would you say it is required to have a separate PR for updating package-lock files?
• When do we update the minified file?
• How are we doing reviews? The duration of the provider improvements reviews and implementation are taking way too long from my point of view. (soon 4 months)

[nivida]

@cgewecke btw.:
Could you please start to use the labels we have and to label those issues and your PRs correctly :)

Edit:

Continuing in the spirit of #3303

Is it possible to first finish PR #3303 before we are creating more such PRs? And is it not also possible to comment in the initial PR and I will do those changes as in a normal review process? It will just get harder to follow anything if we create more PRs. We have moved already PRs together to have a better overview of those depending changes.

[cgewecke]

Would you say it is required to have a separate PR for updating package-lock files?

I think if a dependency is changed in a module, the package-lock should also be changed. But otherwise it should not be included. In #3190 every module had its package-lock changed.

When do we update the minified file?

In a separate PR - maybe as part of the publication process? It's common to see a build step included as a prepublish directive in the npm package.

The duration of the provider improvements reviews and implementation are taking way too long from my point of view. (soon 4 months)

Ah! There's a document describing how to do PRs and Reviews here!

Some parts relevant to #3190 contributing to the long delay and many (unwelcome?) review recommendations are:

• any ... logic change should include tests
• have a narrow, well-defined focus.
• make the smallest set of changes possible to achieve their goal.
• preserve the conventions and stylistic consistency of any files they modify.
• Given the choice between a conservative change that mostly works and an adventurous change which seems better but introduces uncertainty - prefer the conservative change.

[cgewecke]

Could you please start to use the labels we have and to label those issues and your PRs correctly :)

Apologies, I will do this.

Is it possible to first finish PR #3303 before we are creating more such PRs

Can you merge #3303? It just clarifies the test file and now we know that there are test failures when resetting the provider....that's under investigation.

[nivida]

In #3190 every module had its package-lock changed.

This was probably the case because I've changed the dependency graph for the TS improved I've applied which sadly were out of scope.

In a separate PR - maybe as part of the publication process? It's common to see a build step included as a prepublish directive in the npm package.

Sounds good to me!

Ah! There's a document describing how to do PRs and Reviews here!

True that. I think we should just not loos us in over correct PRs. Bigger out of scope changes as you have for example detected here is definitely something I have to pretend but sometimes do we save 15min - 30min if we just keep smaller improvements instead of doing them twice just because of the correctness. Good examples are improving a funcDoc, adding an explanatory in-line comment while going through a code that isn't in the scope, or increasing of the code style quality in a file you anyways have changed.

[nivida]

Can you merge #3303? It just clarifies the test file and now we know that there are test failures when resetting the provider....that's under investigation.

Yep! I will check this PR again and merge it if anything is fine 👌