Issue 29730656: Issue 6476 - Update adblockplusui dependencies to ead38c2013b5 (Closed)
|
Created:
March 22, 2018, 8:06 p.m. by saroyanm Modified:
March 23, 2018, 4:24 p.m. Visibility:
Public. |
DescriptionIssue 6476 - Update adblockplusui dependencies tov ead38c2013b5
Patch Set 1 : #
Total comments: 17
Patch Set 2 : Addressed comments #Patch Set 3 : Updated dependency to ead38c2013b5 #Patch Set 4 : updated git dependency #
MessagesTotal messages: 27
This is ready for the review. I'll update the dependency part as soon #6510 is ready
https://codereview.adblockplus.org/29730656/diff/29730664/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29730656/diff/29730664/lib/options.js#newc... lib/options.js:120: let {name, sender} = port; Nit: I wouldn't care to destructure the port here, and just use port.name and port.sender below. https://codereview.adblockplus.org/29730656/diff/29730664/lib/options.js#newc... lib/options.js:121: if (name != "ui" || !sender || sender.tab.id != tab.id) Under which circumstances do we need to check for !sender? https://codereview.adblockplus.org/29730656/diff/29730664/lib/options.js#newc... lib/options.js:213: port.postMessage({ Nit: The indentation is off here.
Have you tested the extension still builds? I ask since you didn't change anything with the build system or metadata here, so the metadata still references adblockplusui/desktop-options.js and adblockplusui/skin/desktop-options.css which no longer exist. https://codereview.adblockplus.org/29730656/diff/29730664/dependencies File dependencies (right): https://codereview.adblockplus.org/29730656/diff/29730664/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:12c59fe0fa0a git:93b2850 Please instead update the dependency to ead38c2013b5 since I will need to make some other changes when updating the dependency on to 12c59fe0fa0a. Also please update the Git revision.
On 2018/03/23 11:04:32, kzar wrote: > Have you tested the extension still builds? > > I ask since you didn't change anything with the build system or metadata here, > so the metadata still references adblockplusui/desktop-options.js and > adblockplusui/skin/desktop-options.css which no longer exist. To provide some context, the build passes through npm install, which triggers the postinstall script, which creates those files and then exits. At least this is what we've been told when we've asked how to trigger bundlings while creating the build. If that's not the case, we need to drop those files from .hgignore and deal with them on each code review. That'd be less than ideal though, but it would surely fix any possible missing file related gotcha.
On 2018/03/23 11:13:16, a.giammarchi wrote: > To provide some context, the build passes through npm install, which triggers > the postinstall script, which creates those files and then exits. > > At least this is what we've been told when we've asked how to trigger bundlings > while creating the build. I see, so you tested that worked in practice right?
On 2018/03/23 11:21:35, kzar wrote: > On 2018/03/23 11:13:16, a.giammarchi wrote: > > To provide some context, the build passes through npm install, which triggers > > the postinstall script, which creates those files and then exits. > > > > At least this is what we've been told when we've asked how to trigger > bundlings > > while creating the build. > > I see, so you tested that worked in practice right? Before pushing the change I'll do final test, but so far were working for me.
https://codereview.adblockplus.org/29730656/diff/29730664/dependencies File dependencies (right): https://codereview.adblockplus.org/29730656/diff/29730664/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:12c59fe0fa0a git:93b2850 On 2018/03/23 11:04:32, kzar wrote: > Please instead update the dependency to ead38c2013b5 since I will need to make > some other changes when updating the dependency on to 12c59fe0fa0a. Also please > update the Git revision. I was thinking to push this -> https://codereview.adblockplus.org/29730617/ first before updating. https://codereview.adblockplus.org/29730656/diff/29730664/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29730656/diff/29730664/lib/options.js#newc... lib/options.js:121: if (name != "ui" || !sender || sender.tab.id != tab.id) On 2018/03/23 00:01:48, Sebastian Noack wrote: > Under which circumstances do we need to check for !sender? I assume because this is optional, but according to the doc. this should be presented on onConnect, I'll remove it. Good point.
On 2018/03/23 11:21:35, kzar wrote: > ...the build passes through npm install... FWIW `npm install` is called when dependencies with a package.json file is updated / cloned, but not when it's already at the right revision. So if I checked out the revision manually, which is the kind of thing I do during development, then the adblockplusui bundle would not be regenerated.
https://codereview.adblockplus.org/29730656/diff/29730664/dependencies File dependencies (right): https://codereview.adblockplus.org/29730656/diff/29730664/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:12c59fe0fa0a git:93b2850 On 2018/03/23 11:26:47, saroyanm wrote: > On 2018/03/23 11:04:32, kzar wrote: > > Please instead update the dependency to ead38c2013b5 since I will need to make > > some other changes when updating the dependency on to 12c59fe0fa0a. Also > please > > update the Git revision. > > I was thinking to push this -> https://codereview.adblockplus.org/29730617/ > first before updating. I will need to make some further changes to adblockpluschrome when updating the adblockplusui dependency to include 12c59fe0fa0a (see #6511). So I can either also include #6510 at the same time, or we can update the dependency a third time to include that. Either way this review isn't blocked by #6510.
On 2018/03/23 11:27:06, kzar wrote: > On 2018/03/23 11:21:35, kzar wrote: > > ...the build passes through npm install... > > FWIW `npm install` is called when dependencies with a package.json file is > updated / cloned, but not when it's already at the right revision. So if I > checked out the revision manually, which is the kind of thing I do during > development, then the adblockplusui bundle would not be regenerated. Bummer. I see two issues here: 1. we don't have greenkeeper in place and we trust semver caret ranges. If I understand correctly, even if a module with ver ^16.1.0 reaches ^16.99, we'll be stuck with last release version, missing out all possible patches/fixes too. 2. we need a hook to bundle so that if `npm install` is skipped, but `npm run` reveals a `bundle` option, or package.json script has a bundle entry, which is also quite common, I believe the builder *should* invoke `npm run bundle` before interacting with needed files. Not sure yet about hot to solve 1, since Greenkeeper AFAIK doesn't interact with Mercurial, but I think point 2 is a reasonable compromise. Thoughts ?
On 2018/03/23 11:40:04, a.giammarchi wrote: > Thoughts ? Well my only thought really is that this kind of logic probably belongs in the buildtools repository.
Ready for the review https://codereview.adblockplus.org/29730656/diff/29730664/dependencies File dependencies (right): https://codereview.adblockplus.org/29730656/diff/29730664/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:12c59fe0fa0a git:93b2850 On 2018/03/23 11:30:03, kzar wrote: > On 2018/03/23 11:26:47, saroyanm wrote: > > On 2018/03/23 11:04:32, kzar wrote: > > > Please instead update the dependency to ead38c2013b5 since I will need to > make > > > some other changes when updating the dependency on to 12c59fe0fa0a. Also > > please > > > update the Git revision. > > > > I was thinking to push this -> https://codereview.adblockplus.org/29730617/ > > first before updating. > > I will need to make some further changes to adblockpluschrome when updating the > adblockplusui dependency to include 12c59fe0fa0a (see #6511). So I can either > also include #6510 at the same time, or we can update the dependency a third > time to include that. Either way this review isn't blocked by #6510. Noted, I've pushed 6510 and did some tests, as it was dependent on current changes. https://codereview.adblockplus.org/29730656/diff/29730664/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29730656/diff/29730664/lib/options.js#newc... lib/options.js:120: let {name, sender} = port; On 2018/03/23 00:01:48, Sebastian Noack wrote: > Nit: I wouldn't care to destructure the port here, and just use port.name and > port.sender below. Done. https://codereview.adblockplus.org/29730656/diff/29730664/lib/options.js#newc... lib/options.js:121: if (name != "ui" || !sender || sender.tab.id != tab.id) On 2018/03/23 11:26:47, saroyanm wrote: > On 2018/03/23 00:01:48, Sebastian Noack wrote: > > Under which circumstances do we need to check for !sender? > > I assume because this is optional, but according to the doc. this should be > presented on onConnect, I'll remove it. > Good point. Done. https://codereview.adblockplus.org/29730656/diff/29730664/lib/options.js#newc... lib/options.js:213: port.postMessage({ On 2018/03/23 00:01:48, Sebastian Noack wrote: > Nit: The indentation is off here. Done.
On 2018/03/23 11:26:41, saroyanm wrote: > On 2018/03/23 11:21:35, kzar wrote: > > On 2018/03/23 11:13:16, a.giammarchi wrote: > > > To provide some context, the build passes through npm install, which > triggers > > > the postinstall script, which creates those files and then exits. > > > > > > At least this is what we've been told when we've asked how to trigger > > bundlings > > > while creating the build. > > > > I see, so you tested that worked in practice right? > > Before pushing the change I'll do final test, but so far were working for me. Have done tests and it works.
https://codereview.adblockplus.org/29730656/diff/29730664/dependencies File dependencies (right): https://codereview.adblockplus.org/29730656/diff/29730664/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:12c59fe0fa0a git:93b2850 On 2018/03/23 12:03:17, saroyanm wrote: > Noted, I've pushed 6510 and did some tests, as it was dependent > on current changes. Please could you update the dependency to ead38c2013b5 instead? I need to make some other changes at the same time as updating the dependency to 12c59fe0fa0a (#6511) and I can include your 12c59fe0fa0a change (#6510) at the same time / we can update the dependency again afterwards.
https://codereview.adblockplus.org/29730656/diff/29730664/dependencies File dependencies (right): https://codereview.adblockplus.org/29730656/diff/29730664/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:12c59fe0fa0a git:93b2850 On 2018/03/23 12:10:38, kzar wrote: > On 2018/03/23 12:03:17, saroyanm wrote: > > Noted, I've pushed 6510 and did some tests, as it was dependent > > on current changes. > > Please could you update the dependency to ead38c2013b5 instead? I need to make > some other changes at the same time as updating the dependency to 12c59fe0fa0a > (#6511) and I can include your 12c59fe0fa0a change (#6510) at the same time / we > can update the dependency again afterwards. Sorry I think I had missunderstood you, done.
Please could you also put the adblockplusui revision in the review subject? https://codereview.adblockplus.org/29730656/diff/29730664/dependencies File dependencies (right): https://codereview.adblockplus.org/29730656/diff/29730664/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:12c59fe0fa0a git:93b2850 On 2018/03/23 12:15:49, saroyanm wrote: > On 2018/03/23 12:10:38, kzar wrote: > > On 2018/03/23 12:03:17, saroyanm wrote: > > > Noted, I've pushed 6510 and did some tests, as it was dependent > > > on current changes. > > > > Please could you update the dependency to ead38c2013b5 instead? I need to make > > some other changes at the same time as updating the dependency to 12c59fe0fa0a > > (#6511) and I can include your 12c59fe0fa0a change (#6510) at the same time / > we > > can update the dependency again afterwards. > > Sorry I think I had missunderstood you, done. That's OK, but please could you update the Git revision as well?
https://codereview.adblockplus.org/29730656/diff/29730664/dependencies File dependencies (right): https://codereview.adblockplus.org/29730656/diff/29730664/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:12c59fe0fa0a git:93b2850 On 2018/03/23 13:09:39, kzar wrote: > On 2018/03/23 12:15:49, saroyanm wrote: > > On 2018/03/23 12:10:38, kzar wrote: > > > On 2018/03/23 12:03:17, saroyanm wrote: > > > > Noted, I've pushed 6510 and did some tests, as it was dependent > > > > on current changes. > > > > > > Please could you update the dependency to ead38c2013b5 instead? I need to > make > > > some other changes at the same time as updating the dependency to > 12c59fe0fa0a > > > (#6511) and I can include your 12c59fe0fa0a change (#6510) at the same time > / > > we > > > can update the dependency again afterwards. > > > > Sorry I think I had missunderstood you, done. > > That's OK, but please could you update the Git revision as well? It should be updated -> https://codereview.adblockplus.org/29730656/diff/29731606/dependencies
On 2018/03/23 13:09:39, kzar wrote: > Please could you also put the adblockplusui revision in the review subject? done.
https://codereview.adblockplus.org/29730656/diff/29730664/dependencies File dependencies (right): https://codereview.adblockplus.org/29730656/diff/29730664/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:12c59fe0fa0a git:93b2850 On 2018/03/23 14:38:39, saroyanm wrote: > On 2018/03/23 13:09:39, kzar wrote: > > That's OK, but please could you update the Git revision as well? > > It should be updated -> > https://codereview.adblockplus.org/29730656/diff/29731606/dependencies Well so far the the adblockplusui dependency is being changed to "hg:ead38c2013b5 git:1669870", the Mercurial revision is what I asked for so I'm happy with that, but the Git revision is not right. It should rather be 0290fb1.
https://codereview.adblockplus.org/29730656/diff/29730664/dependencies File dependencies (right): https://codereview.adblockplus.org/29730656/diff/29730664/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:12c59fe0fa0a git:93b2850 On 2018/03/23 14:50:23, kzar wrote: > On 2018/03/23 14:38:39, saroyanm wrote: > > On 2018/03/23 13:09:39, kzar wrote: > > > That's OK, but please could you update the Git revision as well? > > > > It should be updated -> > > https://codereview.adblockplus.org/29730656/diff/29731606/dependencies > > Well so far the the adblockplusui dependency is being changed to > "hg:ead38c2013b5 git:1669870", the Mercurial revision is what I asked for so I'm > happy with that, but the Git revision is not right. It should rather be 0290fb1. OOps, sorry for that I used the revision for your last change.. Now should be the right one
This change looks good to me now, but I'm not too happy with the new build system. Sebastian what do you think?
LGTM please push this to the master branch. Note: I'm not happy about the new build system introduced in adblockplusui, but I don't want to further delay the release by blocking this.
On 2018/03/23 15:59:24, kzar wrote: > Note: I'm not happy about the new build system introduced in adblockplusui Which part you don't like? We implemented it in a way that your team should not be affected. I understand we had hiccups for this time but I think the issue here is not how we build our own stuff, rather how we improve our release process. It'd be great if we could further tackle this as soon as Thomas is back.
On 2018/03/23 16:12:27, a.giammarchi wrote: > On 2018/03/23 15:59:24, kzar wrote: > > Note: I'm not happy about the new build system introduced in adblockplusui > > Which part you don't like? I've written a list https://issues.adblockplus.org/ticket/6310#comment:8
On 2018/03/23 16:15:24, kzar wrote: > On 2018/03/23 16:12:27, a.giammarchi wrote: > > On 2018/03/23 15:59:24, kzar wrote: > > > Note: I'm not happy about the new build system introduced in adblockplusui > > > > Which part you don't like? > > I've written a list https://issues.adblockplus.org/ticket/6310#comment:8
On 2018/03/23 16:15:24, kzar wrote: > On 2018/03/23 16:12:27, a.giammarchi wrote: > > On 2018/03/23 15:59:24, kzar wrote: > > > Note: I'm not happy about the new build system introduced in adblockplusui > > > > Which part you don't like? > > I've written a list https://issues.adblockplus.org/ticket/6310#comment:8 I'm not in CC there, thanks for sharing the link!
On 2018/03/23 16:17:18, a.giammarchi wrote: > I'm not in CC there, thanks for sharing the link! Oh yea, you should probably assign that issue to yourself since you already landed a commit for it, or if not add yourself to Cc at least. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
