closure-library icon indicating copy to clipboard operation
closure-library copied to clipboard

remove usages of deprecated goog.structs.Map from closure library

Open ikabirov opened this issue 8 years ago • 7 comments

closure/goog/datasource/datamanager.js closure/goog/debug/tracer.js closure/goog/dom/forms.js closure/goog/i18n/uchar/remotenamefetcher.js closure/goog/labs/net/webchannel/webchannelbase_test.js closure/goog/labs/pubsub/broadcastpubsub_test.js closure/goog/labs/structs/map_perf.js closure/goog/net/browserchannel_test.js closure/goog/net/xhrio.js closure/goog/net/xhriopool_test.js closure/goog/net/xhrmanager.js closure/goog/storage/mechanism/ieuserdata.js closure/goog/storage/storagetester.js closure/goog/structs/linkedmap.js closure/goog/structs/map_test.js closure/goog/structs/set.js closure/goog/structs/structs_test.js closure/goog/testing/asserts_test.js closure/goog/testing/loosemock.js closure/goog/testing/mockstorage.js closure/goog/testing/net/xhrio.js closure/goog/testing/storage/fakemechanism.js closure/goog/ui/dialog.js closure/goog/ui/media/flashobject.js closure/goog/ui/popupmenu.js closure/goog/uri/uri.js

ikabirov avatar Jul 13 '17 11:07 ikabirov

This is definitely a "help wanted" sort of issue, though a number of these will be difficult due to internal usages.

shicks avatar Jul 14 '17 07:07 shicks

Maybe you should use @suppress {deprecated}?

ikabirov avatar Jul 14 '17 14:07 ikabirov

That's a good idea. We actually hide the warnings during compilation internally (preferring to instead expose them during code review) so we don't have as much visibility into that. Feel free to put together a PR, since this is a lower priority item for us and we may not get to it very soon.

shicks avatar Jul 18 '17 17:07 shicks

So, PR with goog.structs.Map replaced with ES6 Map (and Set) will be accepted or it can break some internal CL usage and we should start with @suppress {deprecated} instead?

anwerso avatar Sep 13 '17 13:09 anwerso

For now the best bet is probably to suppress it. Internally we're working on infrastructure to be able to add ES6 code into existing files, but that won't likely finish until end of year (we have a lot of older projects with development serving systems that don't transpile). Once that's done, then the first place to start migrating would be Closure Library-internal usages, that don't bleed into the API. For instance, a particularly hairy usage is goog.ui.dialog, which subclasses goog.structs.Map. Changing this to use an ES6 Map is a significantly incompatible API change, so it's going to be a lot of work to migrate existing usages.

shicks avatar Sep 15 '17 21:09 shicks

I don't see a reason to deprecate goog.structs.Map/Set without codebase refactoring in a first place.

Right now I've just suppressed it in my Closure Library branch in places I use. PR is an other story, @suppress {deprecated} working mysteriously. For example I couldn't find how to suppress it in code like goog.ui.IdleTimer.defaultActivityMonitorReferences_ = new goog.structs.Set();. Only solution I found is suppress deprecated code in whole file (@fileoverview section), which is not an ideal solution.

anwerso avatar Sep 18 '17 15:09 anwerso

The reason is basically internal hemorrhaging. We needed to stop the bleeding. We'd tried other means to communicate "don't use this" and none of them were effective enough - we were still seeing multiple new usages of these types every week, in code that should have been using the ES6 classes. Internally, we don't recommend people compile with deprecated warnings enabled, and instead surface the warnings in our IDEs, code review, and source viewing tools. So it's simultaneously more obvious and less intrusive. We do want to migrate away, but we simply couldn't wait until Closure was migrated before deprecating, since every week we waited, the harder any migration was going to be.

shicks avatar Sep 20 '17 00:09 shicks