Skip to content

Conversation

@jgaehring
Copy link
Contributor

As I mentioned in #15 (comment), I decided to split the Observer changes into a separate PR and npm version, since the hotfixes in alpha-10 really need to be released no matter what. The Observer is in good shape, too, but it does introduce minor changes to the API, so it would give us a safe rollback point if push came to shove.

@jgaehring
Copy link
Contributor Author

Please note that I just force-pushed a change that adds the "git+" to the package.json's repository URL, so that now it is "git+https://github.com/datafoodconsortium/connector-typescript.git". NPM complained that the "git+" prefix was missing when I tried publishing alpha.10 and alpha.11, so I updated it for both releases on the NPM registry. I'll force-push or add another commit to datafoodconsortium/connector-codegen#31 with the updated package.json and package-lock.json as well, so we can be as consistent as possible.

Copy link
Member

@lecoqlibre lecoqlibre left a comment

Choose a reason for hiding this comment

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

I found this implementation is a little complicated. I was thinking about something much more simpler like I said in a previous comment.

For me it was just about adding some callback registering methods like registerCallbackForExport and registerCallbackForImport to the Connector class. Store the callbacks in properties like private callbacksForExport: Set<ExportCallback>; and private callbackForExport: Set<ImportCallback>;. Then call the previously registered callbacks in the Connector:export and Connector:import methods.

I think it would be better to trigger the callbacks in the Connector class rather than in the ConnectorImporterJsonldStream/ConnectorExporterJsonldStream as parsers and serializers should just parse and export.

@jgaehring @RaggedStaff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To review ☕

Development

Successfully merging this pull request may close these issues.

2 participants