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.

Sign up for GitHub

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

Jump to bottom

Remove Object.values() #1089

Merged
merged 1 commit into from
Mar 31, 2018
Merged

Remove Object.values() #1089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion tensorboard/components/tf_imports/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,20 @@ load("//tensorboard/defs:web.bzl", "tf_web_library")

licenses(["notice"]) # Apache 2.0

# TODO(jart): Update @io_bazel_rules_closure//closure/library to export
# fine-grained build rules, so we won't schlep in that whole
# codebase when we're not running the Closure Compiler. Also
# note that TensorBoard build tooling auto-includes base.js.
tf_web_library(
name = "closure",
srcs = [
"closure.html",
"@com_google_javascript_closure_library//:closure/goog/object/object.js",
],
path = "/tf-imports",
visibility = ["//visibility:public"],
)

tf_web_library(
name = "webcomponentsjs",
srcs = ["@org_definitelytyped//:webcomponents.js.d.ts"],
Expand Down Expand Up @@ -491,4 +505,3 @@ genrule(
outs = ["d3-transition.d.ts"],
cmd = "sed '/^declare module/d' $< | awk '/^}$$/ && !p {p++;next}1' >$@",
)

18 changes: 18 additions & 0 deletions tensorboard/components/tf_imports/closure.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<!--
@license
Copyright 2006 The Closure Library Authors. All Rights Reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS-IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->

<script src="closure/goog/object/object.js"></script>
1 change: 1 addition & 0 deletions tensorboard/components/tf_tensorboard/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ tf_web_library(
"//tensorboard/components/tf_backend",
"//tensorboard/components/tf_dashboard_common",
"//tensorboard/components/tf_globals",
"//tensorboard/components/tf_imports:closure",
"//tensorboard/components/tf_imports:lodash",
"//tensorboard/components/tf_imports:polymer",
"//tensorboard/components/tf_paginated_view",
Expand Down
3 changes: 2 additions & 1 deletion tensorboard/components/tf_tensorboard/tf-tensorboard.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
<link rel="import" href="../tf-backend/tf-backend.html">
<link rel="import" href="../tf-dashboard-common/tensorboard-color.html">
<link rel="import" href="../tf-globals/tf-globals.html">
<link rel="import" href="../tf-imports/closure.html">
<link rel="import" href="../tf-imports/lodash.html">
<link rel="import" href="../tf-paginated-view/tf-paginated-view-store.html">
<link rel="import" href="../tf-storage/tf-storage.html">
Expand Down Expand Up @@ -400,7 +401,7 @@ <h3>There’s no dashboard by the name of “<tt>[[_selectedDashboard]]</tt>.”
_dashboardData: {
type: Array,
readOnly: true,
value: Object.values(tf_tensorboard.dashboardRegistry),
value: goog.object.getValues(tf_tensorboard.dashboardRegistry),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can actually just use lodash's _.values method and avoid an import.

That being said, I'm a fan of the Closure library in terms of the multi-browser support, security, and built-in constructs it offers. I'd be up for importing Closure if you think we can make good use of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can absolutely get value out of the Closure Library.

Lodash is actually blocking us from integrating a stack overflow fix in dagrejs/dagre#228 since the release came included with a Lodash surprise. There's maybe a dozen teams at Alphabet who use Lodash. So we're going to need to roll up our sleeves and use Rosie to migrate their codebases, before we can get the benefits of that fix.

},

/**
Expand Down