Skip to content

Support for imports#86

Merged
mafintosh merged 1 commit intomafintosh:masterfrom
martinheidegger:imports
Mar 23, 2022
Merged

Support for imports#86
mafintosh merged 1 commit intomafintosh:masterfrom
martinheidegger:imports

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.

@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 martinheidegger force-pushed the imports branch 2 times, most recently from f0e3bb2 to b86a89e Compare March 23, 2022 02:45
@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.

2 participants