solc-js icon indicating copy to clipboard operation
solc-js copied to clipboard

Refactor wrapper.ts to improve maintainability

Open stephen-slm opened this issue 4 years ago • 2 comments

Why?

This change works on refactoring out the wrapper.ts file to allow improved maintainability, extensibility with support of easy typing. The current implementation is one continuous function call which allocates all the bindings with hard to follow scoping.

Any bindings of methods have been split out into the binding directly, currently containing the core (anything not compiler related) and compiler related bound functions.

Whats Next?

Since the wrapper is the main entry point for most consumers of the service, this should allow cleanly defined types to be exported with the package. Allowing language servers and IDEs to consume the types and expose them to the user for an improved experience.

  • The callback implementation to apply changes based on some additional input arguments can now be tackled as it can be harder to follow. I was going to do it in this PR but it was outside of the scope.

This has been done here #615

stephen-slm avatar Mar 31 '22 07:03 stephen-slm

Coverage Status

Coverage increased (+0.8%) to 84.537% when pulling 864af418384e81dd76bdcf85715a047b8d21c19d on stephensli:refactor/wrapper into fe752205989d6216f07609b390a88fd361d1ff0c on ethereum:master.

coveralls avatar Mar 31 '22 08:03 coveralls

Also, please rebase on latest master. My test PRs went in recently.

cameel avatar Jul 05 '22 12:07 cameel

Hey, what is missing in this PR to be merged? Just the rebase on the latest master? It would be great to have this merged.

r0qs avatar Sep 15 '22 10:09 r0qs

Basically rechecking the 3 comments I mentioned and rebasing. We're pretty much done here.

cameel avatar Sep 15 '22 11:09 cameel

Then we need to rebase #615 on top of that and review.

cameel avatar Sep 15 '22 11:09 cameel

@r0qs and @cameel sorry guys, I have been super busy at work but its now on my plan to complete this on the weekend!

stephen-slm avatar Sep 15 '22 14:09 stephen-slm

Sounds good! I cannot merge it but ping me when its done and i'll rebase the next PR 🔥

stephen-slm avatar Sep 20 '22 18:09 stephen-slm

Merged.

cameel avatar Sep 20 '22 20:09 cameel