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

Remove DesmoldSDK service #16

Closed
wants to merge 4 commits into from
Closed

Conversation

iosonopersia
Copy link
Contributor

It looks like the DesmoldSDK singleton service, apart from being just a useless wrapper over the SDK library, causes some circular dependency errors (only within Firefox!).

I wasn't able to understand why such errors appear when using Firefox but, since that service isn't really needed, I decided to solve the problem by removing it and replacing it with direct calls to the SDK library.

@iosonopersia iosonopersia requested a review from relu91 September 19, 2022 16:03
Copy link
Contributor

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

I'm approving the PR but I would wait for input also from @hyperloris. Services are a nice abstraction layer to avoid that SDK updates will impact a lot of other places (as you can see in this PR the service is imported in different other scripts). I see your rationale but let me think more about it.

@hyperloris
Copy link

I agree with @relu91. I usually prefer to wrap the SDK inside a service exposing only what I need. That way, it becomes easier to handle any "breaking changes" or some extra logic you may need. Let's say that, in this specific case, we can make an exception and use our SDK directly.

@iosonopersia
Copy link
Contributor Author

Please, don't merge, I'm working on this

@iosonopersia
Copy link
Contributor Author

Closing. The Firefox bug stopped appearing and the DesmoldSDK service will be needed for a future PR related to a better integration with the Metamask browser extension.

@iosonopersia iosonopersia deleted the removeDesmoldSDKService branch September 25, 2022 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants