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

Fix js generator for commonjs_strict #8696

Closed
wants to merge 1 commit into from

Conversation

eKazim
Copy link

@eKazim eKazim commented Jun 4, 2021

This resolves js issues when generator used with import_style=commonjs_strict.
The problem was in js-exports that are have nested namespaces. For comparison, when using import_style=commonjs, there is no addiontal nesting in exported objects. So, now js-export is unified, and the use of imported modules is fixed.
This fixes protocolbuffers/protobuf-javascript#40.
Initially, the incorrect generation was made by a commit 13f94b4. In current pr this commit is partially reverted with the correction of incorrect tests.

@google-cla
Copy link

google-cla bot commented Jun 4, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Jun 4, 2021
@eKazim
Copy link
Author

eKazim commented Jun 4, 2021

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Jun 4, 2021
@Ty3uK
Copy link

Ty3uK commented Jun 23, 2021

@lukesandberg, review PR, please

@eKazim
Copy link
Author

eKazim commented Jul 7, 2021

Can someone review this PR please?
And I can't add the required labels (release notes: yes, javascript) cause I do not have permission.

@jenseng
Copy link

jenseng commented Jul 7, 2021

Would love to see this reviewed/merged!

In the meantime I found a workaround, but it only works is your protos don't reference each other, so YMMV. For example, if you have a standalone messages.proto like so:

syntax = "proto3";
package foo.bar.v1;

message A {
  string value = 1;
}

You can make a shim module messagesHack.ts like so:

// export both the types and the implementation 
export * from './messages';
// override the exported implementation
module.exports = require('./messages').foo.bar.v1;

And then import/use stuff from messagesHack like you would normally import messages

@avm99963
Copy link

Sorry, I added a comment here but I confused this PR with PR 8864. I apologize for the confusion!

@eKazim
Copy link
Author

eKazim commented Feb 4, 2022

Any updates of this?

@acozzette
Copy link
Member

We are closing all open JavaScript pull requests because the JavaScript implementation is moving to a new repository: https://github.com/protocolbuffers/protobuf-javascript Sorry for the inconvenience but please feel free to reopen the pull request in the new repo.

@acozzette acozzette closed this May 16, 2022
@eKazim
Copy link
Author

eKazim commented May 16, 2022

@acozzette, Why was it necessary to separate Javascript into a separate repository? Am I right to guess that there is no maintainers for support this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Js TypeError when generated with commonjs_strict
6 participants