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

chore(deps): bump protobufjs & tape patch versions. Fix load of .proto files #66

Merged
merged 4 commits into from
Feb 9, 2024

Conversation

dependabot[bot]
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Feb 8, 2024

Bumps the patch-level group with 2 updates: protobufjs and tape.

Updates protobufjs from 7.2.5 to 7.2.6

Release notes

Sourced from protobufjs's releases.

protobufjs: v7.2.6

7.2.6 (2024-01-16)

Bug Fixes

  • report missing import properly in loadSync (#1960) (af3ff83)
Changelog

Sourced from protobufjs's changelog.

7.2.6 (2024-01-16)

Bug Fixes

  • report missing import properly in loadSync (#1960) (af3ff83)
Commits

NOTE: this release includes https://github.com/protobufjs/protobuf.js/pull/1960. The fix surfaced another issue https://github.com/protobufjs/protobuf.js/issues/1971. To be able to load the files there is a change in `proto.js` file that workarounds the issue. The code will be reverted once https://github.com/protobufjs/protobuf.js/issues/1971 is adressed.


Updates tape from 5.7.2 to 5.7.4

Changelog

Sourced from tape's changelog.

v5.7.4 - 2024-01-24

Fixed

Commits

  • [Deps] update has-dynamic-import 1e50cb3

v5.7.3 - 2024-01-12

Commits

  • [Refactor] Test: cleaner at logic af4d109
  • [Fix] intercept: give a proper error message with a readonly Symbol property 4640a91
  • [Refactor] getHarness: avoid mutating opts, account for only one internal callsite for createExitHarness 19cfc8f
  • [Tests] Spawn processes during tests using execPath so that the tests pass on windows 4a57fbe
  • [Fix] createHarness: when no conf is provided, only should not throw 8a1cccc
  • [Fix] bin/tape: ignore options on windows a2b74f9
  • [Refactor] _assert: avoid reassigning arguments dc64c08
  • [Refactor] Results: use this instead of self 5f831b4
  • [Performance] avoid the extra call frame to new it 78fd0d6
  • [Dev Deps] update aud, npmignore ceabd99
  • [Tests] fix npm test on windows bcf6ce7
  • [Fix] stack trace path parsing on windows 9cbae8a
  • [Refactor] Results createStream: clean up _push handler 878a500
  • [Refactor] Test: a more precise check f6d30cf
  • [Deps] update object.assign 201e650
  • [Tests] ensure the import tests spawn properly d1987c0
  • [actions] skip engines check since bin/tape and the rest of the lib conflict 19af506
  • [Deps] update deep-equal 5d26485
  • [Deps] update mock-property d90c29a
  • [meta] add sideEffects flag 85f593b
Commits
  • 22befd6 v5.7.4
  • 6a5df50 [Fix] handle native ESM URLs in at:
  • 1e50cb3 [Deps] update has-dynamic-import
  • 56569c3 v5.7.3
  • 85f593b [meta] add sideEffects flag
  • 5f831b4 [Refactor] Results: use this instead of self
  • f6d30cf [Refactor] Test: a more precise check
  • dc64c08 [Refactor] _assert: avoid reassigning arguments
  • 878a500 [Refactor] Results createStream: clean up _push handler
  • 19cfc8f [Refactor] getHarness: avoid mutating opts, account for only one internal...
  • Additional commits viewable in compare view

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
  • @dependabot ignore <dependency name> major version will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself)
  • @dependabot ignore <dependency name> minor version will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself)
  • @dependabot ignore <dependency name> will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself)
  • @dependabot unignore <dependency name> will remove all of the ignore conditions of the specified dependency
  • @dependabot unignore <dependency name> <ignore condition> will remove the ignore condition of the specified dependency and ignore conditions

@dependabot dependabot bot added the dependencies Pull requests that update a dependency file label Feb 8, 2024
@dependabot dependabot bot requested a review from a team February 8, 2024 16:37
@trentm trentm assigned trentm and david-luna and unassigned trentm Feb 8, 2024
@dependabot dependabot bot force-pushed the dependabot/npm_and_yarn/patch-level-cbf6eec550 branch from eb57d82 to 03b2282 Compare February 8, 2024 18:22
Bumps the patch-level group with 2 updates: [protobufjs](https://github.com/protobufjs/protobuf.js) and [tape](https://github.com/ljharb/tape).


Updates `protobufjs` from 7.2.5 to 7.2.6
- [Release notes](https://github.com/protobufjs/protobuf.js/releases)
- [Changelog](https://github.com/protobufjs/protobuf.js/blob/master/CHANGELOG.md)
- [Commits](protobufjs/protobuf.js@protobufjs-v7.2.5...protobufjs-v7.2.6)

Updates `tape` from 5.7.2 to 5.7.4
- [Changelog](https://github.com/ljharb/tape/blob/master/CHANGELOG.md)
- [Commits](tape-testing/tape@v5.7.2...v5.7.4)

---
updated-dependencies:
- dependency-name: protobufjs
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: patch-level
- dependency-name: tape
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: patch-level
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot dependabot bot force-pushed the dependabot/npm_and_yarn/patch-level-cbf6eec550 branch from 03b2282 to 4c2563a Compare February 8, 2024 18:22
@david-luna
Copy link
Member

The error was there from the beginning but it wasn't reported. The latest release surfaced it with the following PR
protobufjs/protobuf.js#1960

As a summary, when calling protobuf.loadSync('file.proto', root)

  • reads and parses the proto file
  • detects imports and tries to load them
  • but takes the file directory as the root for a lookup

This error was not reported in v7.2.5 allowing us to load the files individually. Now with the error report the process stops whenever it's parsing a file with imports.

@david-luna
Copy link
Member

I've added an issue to protobufjs repo protobufjs/protobuf.js#1971
There is an alternative which is to make all imports within .proto files relative the the current file path. This will make the update process a bit more complex but we won't need to patch protobuf

Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Please update the PR/commit title and content to mention the loading change.


// TODO: for now `proto` files are copied from
// https://github.com/open-telemetry/opentelemetry-proto
// https://github.com/open-telemetry/opentelemetry-proto/releases/tag/v1.0.0
Copy link
Member

Choose a reason for hiding this comment

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

Are we really using v1.0.0?
I thought it looked like opentelemetry-js.git was using a pre-1.0.0 tag version of the protos. I've no idea if it matters, however.

Copy link
Member

Choose a reason for hiding this comment

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

It should be compatible but I'll sync up in a follow up PR

path = prefix + path.replace(folder, '');
}
return path;
};
Copy link
Member

Choose a reason for hiding this comment

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

Nice. Was the intent to move away from this hack eventually once we have a process to build/commit a proto.json and load that instead?

Copy link
Member

Choose a reason for hiding this comment

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

yes! I think as part of the process of updating the proto files we could change the imports to use relative paths until protobufjs/protobuf.js#1971 is fixed

Copy link
Member

Choose a reason for hiding this comment

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

Would it possibly work to add a mostly-empty root.proto file at the top-level dir, load that first, then use its returned root for loading all the other .proto files?

root.resolvePath = function patchResolvePath(filename) {
let path = Root.prototype.resolvePath.apply(root, arguments);
if (filename) {
const folder = filename.split('/').slice(0, -1).join('/');
Copy link
Member

Choose a reason for hiding this comment

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

nit: Woe be the first Windows user. ;)

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the heads up :)

Copy link
Member

Choose a reason for hiding this comment

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

now using path.resolve() instead of working with strings

@david-luna david-luna changed the title chore(deps): bump the patch-level group with 2 updates chore(deps): bump protobufjs & tape patch versions. Fix load of .proto files Feb 9, 2024
@david-luna david-luna merged commit cd9d4f0 into main Feb 9, 2024
11 checks passed
@david-luna david-luna deleted the dependabot/npm_and_yarn/patch-level-cbf6eec550 branch February 9, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants