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

Treat absolute paths as relative #1863

Closed
ddimitrioglo opened this issue Feb 2, 2023 · 5 comments
Closed

Treat absolute paths as relative #1863

ddimitrioglo opened this issue Feb 2, 2023 · 5 comments

Comments

@ddimitrioglo
Copy link

ddimitrioglo commented Feb 2, 2023

protobuf.js version: 7.2.0

We have a package @packaly/core-sdk package where we store all the proto files and generated files.
After the last dependencies reinstallation, seems that the path resolution got broken.
After investigation, I could drill down to protobufjs which is an indirect dependency for our package.

The structure:

├── package.json
└── packaly
    ├── backend
    │   └── v1
    │        ...
    │       ├── store.pb.js
    │       ├── store.pb.ts
    │       └── store.proto
    ├── common
    │    ...
    │   ├── geo.pb.js
    │   ├── geo.pb.ts
    │   └── geo.proto
    ...

The store.proto

syntax = "proto3";

package packaly.backend.v1;

import "packaly/common/geo.proto"; // <== THIS import fails

service StoreService {
  // some rpcs here
}

with version 7.2.0 (which is installed automatically via the dependency tree) I get the following error:

ENOENT: no such file or directory, open '/MY-PROJECT/node_modules/@packaly/core-sdk/packaly/backend/v1/packaly/common/geo.proto'
Error: The invalid .proto definition (file not found)

when I update my package.json with "protobufjs": "7.1.2" everything works as expected.

@alexander-fenster
Copy link
Contributor

alexander-fenster commented Feb 2, 2023

I also see this problem, I'll see which change breaks the world and will fix / revert.

@alexander-fenster
Copy link
Contributor

v7.2.1 should fix it.

@ddimitrioglo
Copy link
Author

✅ confirm!

Thanks for the quick fix!

@alexander-fenster
Copy link
Contributor

I actually suspect that if your code failed on v7.2.0, it might mean that the imports in your protos might actually be wrong, and the fix I reverted just made the loading code fail loudly where it previously accepted the wrong inputs. It does not justify the breaking change – hence I reverted #1817 – but it might signal that your protos are non compliant. Mine definitely were: I had an incorrect relative path in the import directive. So, if you were affected by this, it's worth checking your imports :)

@ddimitrioglo
Copy link
Author

@alexander-fenster I've posted the structure and import above, so if you see any issues I'd be more than happy to fix them if you provide any hints on how to do that...

P.S. Currently, I'm using buf lint to validate protos and generate the code and it passes validation with my existing structure.

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

No branches or pull requests

2 participants