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

Skip exports if not available by CommonJS #8856

Merged
merged 1 commit into from Oct 14, 2021

Conversation

Danielku15
Copy link
Contributor

@Danielku15 Danielku15 commented Aug 3, 2021

Resolves #8855

This is a minimal fix for the problem reported in the above issue. I added a simplistic check whether the exports variable is an object to be filled. This eliminates the error without making big changes to really extend to an UMD. Proper UMD support (also in the generated code) would likely need a bigger change in code consisting of changes in the file structure, code generator (protoc), tests etc.

My primary goal is to eliminate the browser error.

@google-cla
Copy link

@google-cla google-cla bot commented Aug 3, 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 Aug 3, 2021
@google-cla
Copy link

@google-cla google-cla bot commented Aug 3, 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.

@Danielku15
Copy link
Contributor Author

@Danielku15 Danielku15 commented Aug 3, 2021

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Aug 3, 2021
@acozzette acozzette requested a review from lukesandberg Aug 3, 2021
@elharo elharo added javascript release notes: no release notes: yes and removed release notes: no labels Aug 24, 2021
@Danielku15
Copy link
Contributor Author

@Danielku15 Danielku15 commented Sep 6, 2021

@lukesandberg @acozzette Any chance this will get reviewed and integrated in the near future. Otherwise I we will need likely to either patch locally the library or fully replace it with some other lib. 😢

@Danielku15
Copy link
Contributor Author

@Danielku15 Danielku15 commented Sep 21, 2021

We will try to keep our response time within 7-days but if you don’t get any response in a few days, feel free to comment on the threads to get our attention. (https://github.com/protocolbuffers/protobuf/blob/master/CONTRIBUTING.md)

@lukesandberg @acozzette
It has been now quite a while since I reported this issue and provided a PR to fix it. I saw some activities on the labels but beside that the replies are rather slient 😢 Since quite some sprints were are now shifting an open bug report on our side related to this page error. I would appreciate any feedback on whether you plan to integrate this change or whether we are better set with a different framework.

@Danielku15
Copy link
Contributor Author

@Danielku15 Danielku15 commented Oct 11, 2021

Ping @lukesandberg @acozzette Is this project still active?

@acozzette acozzette merged commit 3e02f65 into protocolbuffers:master Oct 14, 2021
55 checks passed
@acozzette
Copy link
Member

@acozzette acozzette commented Oct 14, 2021

@Danielku15 Sorry for the delay and thank you for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes javascript release notes: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants