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 prototype package dependency #22

Closed
wants to merge 1 commit into from

Conversation

jstotz
Copy link

@jstotz jstotz commented Sep 7, 2016

Hi Clover team,

Not sure if you want this PR but because of the reasons described in #16 we
were unable to use the SDK with the prototype package as a dependency.

This change:

  • Extracts the Class implementation from the prototype package as a
    standalone file that does not modify any globals (see Class.js).
  • Updates README to describe new way of extending classes
  • Manually patches the files generated by Avro. If someone wants
    to point me at the code that generates these files from the Avro schemas
    I'd be happy to update that, too.

I've tested this change in our app and it appears to work as expected
but we certainly don't exercise the entire SDK codebase.

Resolves #16

- Extracts the Class implementation from the prototype package as a
  standalone file that does not modify any globals (see Class.js).

- Updates README to describe new way of extending classes

NOTE: This manually patches the files generated by Avro to show how
they'll need to be generated.

Resolves clover#16
@mike-clover-com
Copy link
Contributor

Hi Jay. Ironic, but you may see that there is another pull request here to remove this dependency - opened shortly after yours.

Please feel free to take a look at the other pull request. It is closely tied to https://github.com/clover/remote-pay-cloud-api, a new repository that breaks out the api classes (and also avoids the prototype.js dependency). There are also three other repositories/pull requests that may be of interest - https://github.com/clover/remote-pay-cloud-connector-configuration-react/pull/7 and https://github.com/clover/clover-cloud-connector-example/pull/7 are an example application, and https://github.com/clover/clover-cloud-connector-unit-examples/pull/4 is another example application that is useful for testing functionality.

I will take a look at your pull request as well, but I think that we may have this resolved shortly - including appropriate NPM publishings, by the end of this week.

@jstotz
Copy link
Author

jstotz commented Sep 8, 2016

Ha, well that's some timing. Thanks for getting that done. The approach you took in #23 to use plain old constructor functions and prototype manipulation is definitely better. Particularly nice for us since we use ES6 and it's now compatible with class OurListener extends ICloverConnectorListener syntax.

Feel free to close this PR.

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.

2 participants