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

js: protocols bundled with package are incompatible with commonjs_strict #25

Open
ChALkeR opened this issue Aug 5, 2020 · 12 comments
Open
Assignees
Labels
help wanted Extra attention is needed javascript triaged Issue has been triaged

Comments

@ChALkeR
Copy link

ChALkeR commented Aug 5, 2020

This issue is about [email protected] js package, as published on npm.

Currently, it contains the following files:

./README.md
./google-protobuf.js
./package.json
./google/protobuf/any_pb.js
./google/protobuf/empty_pb.js
./google/protobuf/descriptor_pb.js
./google/protobuf/field_mask_pb.js
./google/protobuf/type_pb.js
./google/protobuf/duration_pb.js
./google/protobuf/source_context_pb.js
./google/protobuf/struct_pb.js
./google/protobuf/api_pb.js
./google/protobuf/compiler/plugin_pb.js
./google/protobuf/timestamp_pb.js
./google/protobuf/wrappers_pb.js

./google/ protocols are built in commonjs mode as opposed to commonjs_strict, which makes them both pollute the global scope (they create a global proto variable) and not compatible with CSP (they use Function constructor):

var jspb = require('google-protobuf');
var goog = jspb;
var global = Function('return this')();

goog.exportSymbol('proto.google.protobuf.Any', null, global);
/**
 * Generated by JsPbCodeGenerator.
 * @param {Array=} opt_data Optional initial data array, typically from a
 * server response, or constructed directly in Javascript. The array is used
 * in place and becomes part of the constructed object. It is not cloned.
 * If no data is provided, the constructed object will be empty, but still
 * valid.
 * @extends {jspb.Message}
 * @constructor
 */
proto.google.protobuf.Any = function(opt_data) {

I would have expected commonjs_strict protocols to be present there, either under the same folder, or perhaps in a separate google/protobuf.strict or strict/google/protobuf directory.

That would make them usable in commonjs_strict mode.

@ChALkeR ChALkeR changed the title js: protocols inside bundle are incompatible with commonjs_strict js: protocols bundled with package are incompatible with commonjs_strict Aug 5, 2020
@perezd
Copy link

perezd commented Oct 22, 2020

We'd be happy for you to submit a PR for this!

@dlj-NaN
Copy link
Contributor

dlj-NaN commented Dec 8, 2020

Both protocolbuffers/protobuf#5464 (Dec 13, 2018, 3 thumbs up) and protocolbuffers/protobuf#6770 (Oct 15, 2019, 8 thumbs up) are similar. I have closed them in favor of this issue, since I think @ChALkeR has provided a great staring point here.

x746e referenced this issue in x746e/trackd Mar 8, 2021
Javascript implementation of protobuf uses eval (https://github.com/protocolbuffers/protobuf/issues/7778), which isn't allowed in Chrome extensions v3 manifest.  But we need v3 manifest to use `chrome.tabGroups` API, so I don't see an easy way to use grpc from a Chrome plugin.  And, at the end of the day, we don't really gain that much from grpc in that small kind of project.
@JulesPatry
Copy link

Any update on this?

@arzmir
Copy link

arzmir commented Aug 6, 2021

Seing as @lukesandberg was assigned to this issue. Is it safe to assume there is some traction here @perezd?

@marnixbouhuis
Copy link

marnixbouhuis commented Aug 6, 2021

I've created a PR that fixes this issue partially, protocolbuffers/protobuf#8864 fixes the CSP issue, but does not generate the protobufs in ./google/ with commonjs_strict.

avm99963 referenced this issue in avm99963/protobuf Sep 6, 2021
When generating JS code for .proto files which included well-known types
with the commonjs_strict import style, the generated code would import
generated JS code for the well-known types with the commonjs import
style (located in the google-protobuf NPM package). This causes several
issues as discussed in #7778.

This CL changes this so when using commonjs_strict, the imported
generated JS code for well-known types also uses commonjs_strict.

Fixes #7778
@avm99963
Copy link

avm99963 commented Sep 6, 2021

@marnixbouhuis I've submitted PR protocolbuffers/protobuf#8955 which complements yours, to generate the well-known types code with the commonjs_strict import style :)

@eKazim
Copy link

eKazim commented Sep 14, 2021

It would be nice to merge to master PR protocolbuffers/protobuf#8955 together with the fix of #40 (PR protocolbuffers/protobuf#8696), because otherwise it is possible that it will still be impossible to use this modules generated with commonjs_strict.
But unfortunately, PRs for javascript are ignoring for a long time...

@avm99963
Copy link

But unfortunately, PRs for javascript are ignoring for a long time...

I just noticed your PR is stuck for review since June :(( It'd be awesome if some maintainer could review all the PRs mentioned in this issue! (#8696, protocolbuffers/protobuf#8864, protocolbuffers/protobuf#8955)

otherwise it is possible that it will still be impossible to use this modules generated with commonjs_strict

Could you expand on that? Right now I don't notice how this could happen, so I might be missing something.

@eKazim
Copy link

eKazim commented Sep 14, 2021

Could you expand on that? Right now I don't notice how this could happen, so I might be missing something.

According to the example in the comment , there will be extra nesting of package in the export of the js module. So, when you will try to use the current generated modules in another js module by importing them (require()), it will return undefined instead of the desired structure. But perhaps, this problem is relevant for a deeper nesting of the package, and maybe with package google.protobuf there is no problem. In theory, it is easy to check this by collecting these modules manually and trying to use them in another module.

@dibenede
Copy link
Contributor

dibenede commented Sep 2, 2022

I'm currently looking at some packaging issues and can take a look at this.

@dibenede dibenede added the triaged Issue has been triaged label Sep 2, 2022
@dibenede dibenede assigned dibenede and unassigned lukesandberg Sep 2, 2022
@unicornonea
Copy link

Any update on this issue?

@lukesandberg
Copy link
Contributor

i think it would be reasonable to start releasing the well known types in strict mode to npm, a la protocolbuffers/protobuf#8955

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed javascript triaged Issue has been triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.