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

Experimental Feature: Add ES6 import style #821

Merged
merged 1 commit into from
Jun 4, 2020

Conversation

Yannic
Copy link
Contributor

@Yannic Yannic commented May 26, 2020

This change adds a new import_style that emits ES6 modules.
For now, this only re-exports the symbols from import_style=closure.

In the future, this will no longer require emitting google modules
(goog.provide). If protobuf ever adds support for emitting ES6
modules, we will use them instead of the goog.provided'd versions
as well.

Note that the Bazel integration is currently broken because of a
limitation in rules_closure.

@stanley-cheung
Copy link
Collaborator

stanley-cheung commented May 29, 2020

@Yannic Thanks for the contributions, as always.

There are some build failures. See below. But the bigger issue is that - there is a big change coming from the internal codebase being reviewed at the same time (even coming up the same day!). There was some other issues at hand, which looks like we have to split outputing the normal callback-based client and the Promised-based client into separate files. See #827.

So there will be some conflicts around here: https://github.com/grpc/grpc-web/pull/821/files#diff-2e729f9088e537bfeb6fefc318963a96R1392-R1402. I tried to merge these 2 changes as a test but obviously they have conflicts.

So my plan is to let the internal change finalized first. And then I will export that out (most likely to merge #827), and then can you rebase on top of that? So this may take a while to develop. I will let you know again in this PR when that's ready.

Sorry for the inconvenience.

The build failure:

https://source.cloud.google.com/results/invocations/570b2ff5-c0d5-48db-a494-1d2d25ab8103/targets/grpc%2Fweb%2Fpresubmit/log

Step 16/18 : RUN wget -nv -O buildifier   https://github.com/bazelbuild/buildtools/releases/download/$BUILDIFIER_VERSION/buildifier &&   chmod +x ./buildifier &&   ./buildifier --mode=check --lint=warn --warnings=all -r bazel javascript net &&   rm ./buildifier
 ---> Running in 2e444808854f
2020-05-29 03:30:52 URL:https://github-production-release-asset-2e65be.s3.amazonaws.com/60805080/6e79c580-4910-11ea-9c29-8411a2dd01e4?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20200529%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20200529T033052Z&X-Amz-Expires=300&X-Amz-Signature=10db3b8a00be7b5531ad5298830b4fab71ad3590a037788537b3e1c8743f74d5&X-Amz-SignedHeaders=host&actor_id=0&repo_id=60805080&response-content-disposition=attachment%3B%20filename%3Dbuildifier&response-content-type=application%2Foctet-stream [4729152/4729152] -> "buildifier" [1]
bazel/closure_grpc_web_library.bzl:91: uninitialized: Variable "js" may not have been initialized. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#uninitialized)
bazel/closure_grpc_web_library.bzl:101: uninitialized: Variable "src" may not have been initialized. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#uninitialized)

@Yannic
Copy link
Contributor Author

Yannic commented May 29, 2020

Thanks for the update, sounds good. Let me know when the other change landed and I'll rebase and fix this.

@stanley-cheung
Copy link
Collaborator

@Yannic Hi, #827 is merged and the corresponding internal change seems to be stable (i.e. not rolled back after a few days). So this should be a safe time to rebase this PR off the latest master now. Thanks!

This change adds a new `import_style` that emits ES6 modules.
For now, this only re-exports the symbols from `import_style=closure`.

In the future, this will no longer require emitting google modules
(`goog.provide`). If protobuf ever adds support for emitting ES6
modules, we will use them instead of the `goog.provided`'d versions
as well.

Note that the Bazel integration is currently broken because of a
limitation in `rules_closure`.
@Yannic
Copy link
Contributor Author

Yannic commented Jun 4, 2020

@stanley-cheung Thanks, rebased. Fortunately, the merge conflicts turned out to be trivial (both commits inserted a function in the same location). I've also fixed the buildifier warnings.

If this will be mentioned in the release notes, please make sure it's marked as experimental and that we reserve the right to do breaking changes.

@stanley-cheung stanley-cheung changed the title Add experimental ES6 import style Experimental Feature: Add ES6 import style Jun 4, 2020
@stanley-cheung stanley-cheung merged commit a055960 into grpc:master Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants