Skip to content
This repository was archived by the owner on Jun 15, 2020. It is now read-only.

builder.dust: match v5.0.1 API#13

Merged
fungiboletus merged 1 commit into
SINTEF-9012:masterfrom
kalbasit:master
Aug 8, 2016
Merged

builder.dust: match v5.0.1 API#13
fungiboletus merged 1 commit into
SINTEF-9012:masterfrom
kalbasit:master

Conversation

@kalbasit
Copy link
Copy Markdown
Contributor

@kalbasit kalbasit commented Aug 1, 2016

@fungiboletus fungiboletus merged commit 0ec45e4 into SINTEF-9012:master Aug 8, 2016
@fungiboletus
Copy link
Copy Markdown
Member

Thank you.

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Aug 8, 2016

This needs to be reverted. Buffer is not defined in browser settings, so this change effectively breaks Proto2TypeScript for non-Node environments.

@kalbasit
Copy link
Copy Markdown
Contributor Author

kalbasit commented Aug 8, 2016

Not sure reverting is the right solution here as I only brought the library up to date. Should we source the definition of Buffer instead?

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Aug 8, 2016

No, that would not be correct in non-Node environments.

Note that encodeNB throws in non-Node environments, so it is not expected to be usable from a browser.

@kalbasit
Copy link
Copy Markdown
Contributor Author

kalbasit commented Aug 8, 2016

@tamird true but again, this repo provides the TD of a protobuf, it does not assume NodeJS or Browser. So perhaps we could add a flag to enable node-only definitions?

@yellowiscool we should add node-only and browser-only tests to this repo?

@fungiboletus
Copy link
Copy Markdown
Member

I don't know whether we should revert the commit. The "master" branch has never been very stable.

But we should definitely fix that. The flag to enable the node-only definitions is a good idea. The tests could also be improved, they were develop when I was the single user of the project, and they are far from perfect.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants