New issue
Advanced search Search tips

Issue 898692 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Mojom JS externs generation breaks with multi-file modules

Project Member Reported by rockot@google.com, Oct 24

Issue description

If you have two mojoms that define symbols in the same module, their generated externs JS will be mutually exclusive, since they both provide definitions for the module namespace.

AFAICT Closure does not support merging namespace definitions across multiple files, so we'll have to generate a separate externs file just for each module declaration.
 
Yeah I think we can just emulate goog.provide from Closure base, as this is essentially how Closure module externs deal with this.

Essentially, instead of blindly assigning

  var namespace = {};
  namespace.subnamespace = {};
  // etc...

We manually insert named properties if they're not present in the global namespace or subobjects.
Cc: calamity@chromium.org
Actually... since we should never use externs in prod and they're only generated for feeding to Closure, we can just use goog.provide even without having the Closure base library available. Local tests confirm that this seems to be a pretty good solution.
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1981150801c9d7509ad109e87cddb4e7890eaeca

commit 1981150801c9d7509ad109e87cddb4e7890eaeca
Author: Ken Rockot <rockot@chromium.org>
Date: Thu Oct 25 16:58:09 2018

Use goog.provide to decalare mojom JS symbols

This changes old and new JS bindings to both use goog.provide in their
externs to declare namespaced symbols generated from mojom. This avoids
any conflict between different files declaring different symbols in the
same module namespace, as Closure handles this properly by apparently
special-casing the meaning of goog.provide.

Bug:  898692 
Change-Id: Ib705780a9f0f50f0ff9908dab1debc3dd56fc6f1
Reviewed-on: https://chromium-review.googlesource.com/c/1298638
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602762}
[modify] https://crrev.com/1981150801c9d7509ad109e87cddb4e7890eaeca/mojo/public/tools/bindings/generators/js_templates/externs/interface_definition.tmpl
[modify] https://crrev.com/1981150801c9d7509ad109e87cddb4e7890eaeca/mojo/public/tools/bindings/generators/js_templates/externs/module.externs.tmpl
[modify] https://crrev.com/1981150801c9d7509ad109e87cddb4e7890eaeca/mojo/public/tools/bindings/generators/js_templates/externs/struct_definition.tmpl
[modify] https://crrev.com/1981150801c9d7509ad109e87cddb4e7890eaeca/mojo/public/tools/bindings/generators/js_templates/lite/interface_externs.tmpl
[modify] https://crrev.com/1981150801c9d7509ad109e87cddb4e7890eaeca/mojo/public/tools/bindings/generators/js_templates/lite/module.externs.tmpl
[modify] https://crrev.com/1981150801c9d7509ad109e87cddb4e7890eaeca/mojo/public/tools/bindings/generators/js_templates/lite/struct_externs.tmpl

Status: Fixed (was: Assigned)
Should be good now.

Sign in to add a comment