Closed Bug 1118364 Opened 5 years ago Closed 5 years ago

Remove widget module from SDK

Categories

(Add-on SDK Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla40

People

(Reporter: zer0, Assigned: zer0)

References

Details

(Keywords: addon-compat)

Attachments

(1 file)

widget module is already deprecated since a while, and it's time to get rid of it.
Assignee: nobody → zer0
Duplicate of this bug: 1069524
cc'ing Henrik as well.
Priority: -- → P1
Duplicate of this bug: sdk/widget
Duplicate of this bug: 1115927
This doesn't have a patch yet, haven't heard/seen any progress either, so I'm uplifting without it.
No longer blocks: 1114752
(In reply to Jeff Griffiths (:canuckistani) from comment #2)
> cc'ing Henrik as well.

I have a very hard time at the moment to reserve time for memchaser. :( If someone could help us to get rid of the widget module, I would appreciate. The appropriate PR on our repository is https://github.com/mozilla/memchaser/pull/194. Even comments are welcome. Thanks.
Hey Matteo, are you still working on this?  can someone else take it?
Flags: needinfo?(zer0)
Blocks: 1124161
Blocks: 1124163
Comment on attachment 8552431 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1838

The mapping test, which tests that `require("widget")` maps to `require("sdk/widget")` needs to be changed to make sure this mapping does not occur now, instead of being removed.
Attachment #8552431 - Flags: review?(evold) → review-
See the github comment. Once we agreed on that, I'll push a new commit.
Flags: needinfo?(evold)
Blocks: 1124164
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #10)
> See the github comment. Once we agreed on that, I'll push a new commit.

I responded in github, I think we should have a test for the expected result when one does `require("widget")` with the cuddlefish loader, since we know this is a use case which add-on devs are very likely to hit if they use the module.

Also since you missed removing the mapping, which this test would have caught ;)
Flags: needinfo?(evold)
Hey, I just replied to github as well. Sorry it took me long, I was focused on inspector's stuff and I didn't noticed you've replied – needinfo me next time, so that I won't miss it.

I pushed the PR with the mapping removed, but still the part about testing a non-existing module is not clear to me.
We warned the users every time they required widget with a console.warn that those module was deprecated, so I think we cover the user's story.
Plus we never tested for any module we removed in the past, so I'm not sure what we should test – and moreover, where put those tests. It sounds strange to me having a `test-widget-doesnt-exist.js`; I think we already test a generic non-existing module.
Flags: needinfo?(evold)
The most important ting about this to me is that add-on developers are aware of the removal. ccing Jorge so he sees this. Rip it out!
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #12)
> Hey, I just replied to github as well. Sorry it took me long, I was focused
> on inspector's stuff and I didn't noticed you've replied – needinfo me next
> time, so that I won't miss it.
> 
> I pushed the PR with the mapping removed, but still the part about testing a
> non-existing module is not clear to me.
> We warned the users every time they required widget with a console.warn that
> those module was deprecated, so I think we cover the user's story.

I think this case is different than others before it, and I don't think the fact that we warned users the module was deprecated in the past should affect a decision as to whether or not we should write a test that makes sure the widget module doesn't exist and doesn't have a mapping now.

> Plus we never tested for any module we removed in the past, so I'm not sure
> what we should test

Which high level module have we removed in the past? I'm not aware of one.

> and moreover, where put those tests. It sounds strange
> to me having a `test-widget-doesnt-exist.js`; I think we already test a
> generic non-existing module.

using the test-widget.js and/or test-cuddlefish-loader.js files seems fine to me.

we're testing that the user experience of using an old high level module which is now removed is that which we expect, it helps us make sure we don't miss regressions like the one we might have had if I didn't notice that you didn't make the change needed in mappings.json.
Flags: needinfo?(evold) → needinfo?(zer0)
The replacement for Widget, the "ui" module, is still in "Experimental" status. Until that module is stable, removing Widget is premature.  In fact, deprecating Widget before "ui" was ready was premature.

Ref: https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/ui
Also, there are problems with the "ui" module.
See Bug 1130610. 
It's far too soon to remove "widget".
(In reply to John Nagle from comment #16)
> Also, there are problems with the "ui" module.
> See Bug 1130610. 

Thanks for the tip.

> It's far too soon to remove "widget".

Widget will no longer work with an e10s browser, so the timing is not entirely within our control. Agreed that the above bug should be fixed.
(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #14)

> I think this case is different than others before it, and I don't think the
> fact that we warned users the module was deprecated in the past should
> affect a decision as to whether or not we should write a test that makes
> sure the widget module doesn't exist and doesn't have a mapping now.

I just mentioned that because you said "this is a use case which add-on devs are very likely to hit if they use the module".

> > Plus we never tested for any module we removed in the past, so I'm not sure
> > what we should test

> Which high level module have we removed in the past? I'm not aware of one.

We always removed low level module, but it should not makes difference, because the tests are not intended to be for the end-user. The purpose of the tests are to check that everything works as expected and we do not introduce regression. So I really don't understand which kind of test should be the one you're suggesting.

> > and moreover, where put those tests. It sounds strange
> > to me having a `test-widget-doesnt-exist.js`; I think we already test a
> > generic non-existing module.
> 
> using the test-widget.js and/or test-cuddlefish-loader.js files seems fine
> to me.

Please, if you have hard feeling about that, reply to my comment in github providing the code you have in mind, and I even if I disagree about adding a reference to a non existing old module, I'll add it to my PR.
The `test-cuddlefish-loader.js` it's to me a better candidates for that.

> we're testing that the user experience of using an old high level module
> which is now removed is that which we expect,

I disagree about that. We're testing a non-existing module, adding a reference for something is not even in the code.

> it helps us make sure we don't
> miss regressions like the one we might have had if I didn't notice that you
> didn't make the change needed in mappings.json.

Well, that's what the review are for, right? :) Plus, I think that it would just provide to the user the same user experience – I mean, it maps to a non existing module anyway.
To me, the problem was that I left a reference to something I should removed, not about regression or user experience, in that case.

So, if you can reply to my comment in github, I'll appreciate. Thanks!
Flags: needinfo?(zer0) → needinfo?(evold)
Erik, an alternative, could be modifying the `test-require` tests, to add the widgets on `test_invalid_sdk_module` (or adding another couple of tests that does the same, there), but again, I'm not sure what really you want to test, so maybe that is not what you have in mind.
(In reply to John Nagle from comment #15)

> The replacement for Widget, the "ui" module, is still in "Experimental" status

You raised a good point: I think that we should change that in the code and documentation. The new UI API are not experimental anymore I would say, especially some of them. It's been a while we have them. What do you think, Jeff?
Flags: needinfo?(jgriffiths)
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #19)
> Erik, an alternative, could be modifying the `test-require` tests, to add
> the widgets on `test_invalid_sdk_module` (or adding another couple of tests
> that does the same, there), but again, I'm not sure what really you want to
> test, so maybe that is not what you have in mind.

I think we should remove the python tests, if those are what you are referring to.
Flags: needinfo?(evold)
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #18)
> (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #14)
> > > Plus we never tested for any module we removed in the past, so I'm not sure
> > > what we should test
> 
> > Which high level module have we removed in the past? I'm not aware of one.
> 
> We always removed low level module, but it should not makes difference,
> because the tests are not intended to be for the end-user. The purpose of
> the tests are to check that everything works as expected and we do not
> introduce regression. So I really don't understand which kind of test should
> be the one you're suggesting.

The tests are not for the end user, but the tests are meant to test the user's/dev's experience using our apis, in other words our tests are meant to tests the actual outputs for inputs like `require("widget")` are as expected.  We don't have to test every require input, but we know widget was one used in the past and is not one that should be used in the future, so a test is valuable there.  It helps to make sure that if everyone on the team is gone in 5 years, then the person maintaining the sdk still will know not to add a widget module and why.

In tdd this test is written before removing the module.

> > > and moreover, where put those tests. It sounds strange
> > > to me having a `test-widget-doesnt-exist.js`; I think we already test a
> > > generic non-existing module.
> > 
> > using the test-widget.js and/or test-cuddlefish-loader.js files seems fine
> > to me.
> 
> Please, if you have hard feeling about that, reply to my comment in github
> providing the code you have in mind, and I even if I disagree about adding a
> reference to a non existing old module, I'll add it to my PR.
> The `test-cuddlefish-loader.js` it's to me a better candidates for that.

I don't have a hard feeling about which file we use to store the test at all.  I would like to see this test though, I've provided the two lines of code in the github pull request, they are just:

    assert.throws(() => require("widget"), /.../, "There is no widget module");
    assert.throws(() => require("sdk/widget"), /.../, "There is no sdk/widget module");

> > we're testing that the user experience of using an old high level module
> > which is now removed is that which we expect,
> 
> I disagree about that. We're testing a non-existing module, adding a
> reference for something is not even in the code.

The module is in the addon-sdk repo, you are referring to one state of the repo.  What matters imo is that the module name was used in the past.

> > it helps us make sure we don't
> > miss regressions like the one we might have had if I didn't notice that you
> > didn't make the change needed in mappings.json.
> 
> Well, that's what the review are for, right? :) Plus, I think that it would
> just provide to the user the same user experience – I mean, it maps to a non
> existing module anyway.
> To me, the problem was that I left a reference to something I should
> removed, not about regression or user experience, in that case.
> 
> So, if you can reply to my comment in github, I'll appreciate. Thanks!

responded on github.
Hardware: x86 → All
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #20)
> (In reply to John Nagle from comment #15)
> 
> > The replacement for Widget, the "ui" module, is still in "Experimental" status
> 
> You raised a good point: I think that we should change that in the code and
> documentation. The new UI API are not experimental anymore I would say,
> especially some of them. It's been a while we have them. What do you think,
> Jeff?

Agreed, but I also think we should look into any outstanding issues eg Bug 1130610 as mentioned by John.
Flags: needinfo?(jgriffiths)
Comment on attachment 8552431 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1838

Alright I think we gain more by just landing now and adding the test I wanted later, so go ahead and land it as it is.
Attachment #8552431 - Flags: review- → review+
Attachment #8552431 - Flags: review+ → review?(evold)
Comment on attachment 8552431 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1838

Thanks!
Attachment #8552431 - Flags: review?(evold) → review+
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/3ff5c3187f4485a987ec673876fe4db376b19eb8
Bug 1118364 - Remove widget module from SDK
 - Widget module removed as well as the unit test
 - Widget examples removed

https://github.com/mozilla/addon-sdk/commit/f9bff95f72312a65eaaaa64dddec22a05c612c7b
Merge pull request #1838 from ZER0/remove-widget/1118364

Bug 1118364 - Remove widget module from SDK r=@erikvold
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.