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

Support for imports #86

Merged
merged 1 commit into from Mar 23, 2022
Merged

Support for imports #86

merged 1 commit into from Mar 23, 2022

Conversation

martinheidegger
Copy link
Contributor

Note: based on #85 (standard.js update)

This PR adds tested support for import statements. During the implementation I looked to make sure that it works somehow similar to the specification.

Currently protocol-buffers-schema doesn't support public/weak modifiers **, which means of the 3 ways this could be implemented I chose one. For the simplicity of the PR I went with treating all imports as public. This allowed for a simple/working implementation.

** → PR's to support it in protocol-buffers-schema#66 and ...#67

This PR comes with support for -I and --proto_path in the CLI to specify the source path, tests and documentation.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
compile.js Outdated Show resolved Hide resolved
compile.js Outdated Show resolved Hide resolved
@mafintosh
Copy link
Owner

Import logic looks fine 👍 Major bump due to the var -> consts. See my comments about the indentation changes.

@mafintosh
Copy link
Owner

Remove the use-strict stuff also. That might create unintended behaivor (I doubt it will but also don't have bandwidth to research that). Just move to lts for Node, this means major bumping anyway.

@mafintosh
Copy link
Owner

Other than that good to go.

@martinheidegger
Copy link
Contributor Author

As far as I saw the project is backwards compatible to Node 4 and by using 'use strict', it would stay that way?!

@mafintosh
Copy link
Owner

It's a major bump anyway, so might as well just drop the ancient versions.

@martinheidegger
Copy link
Contributor Author

👍 Do you mind merging #84

@martinheidegger
Copy link
Contributor Author

Tests pass, I think it's ready.

@mafintosh
Copy link
Owner

@martinheidegger thanks! this seems to conflict now that I merged your other PR

@martinheidegger
Copy link
Contributor Author

Rebased.

@mafintosh mafintosh merged commit 1681c94 into mafintosh:master Mar 23, 2022
@mafintosh
Copy link
Owner

🚀

@mafintosh
Copy link
Owner

5.0.0

@martinheidegger martinheidegger deleted the imports branch March 23, 2022 11:19
@martinheidegger
Copy link
Contributor Author

🥳 Thanks a lot! This probably also closes: #45

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.

None yet

2 participants