Issue 29460576: Issue 5079 - Turn elemHideEmulation into a CommonJS module (Closed)
|
Created:
June 9, 2017, 5:08 p.m. by hub Modified:
Aug. 16, 2017, 4:11 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 5079 - Turn elemHideEmulation into a CommonJS module
Applies on top of
https://codereview.adblockplus.org/29448560/
Patch Set 1 #
Total comments: 12
Patch Set 2 : Added the eslintrc #
Total comments: 2
Patch Set 3 : Rebased. No longer need browser env #Patch Set 4 : Updated patch. Fails for now #Patch Set 5 : Rebased on master #
Total comments: 2
MessagesTotal messages: 17
I couldn't find an easy way to do the "require()" in elemHideEmulation. Which mean that for now this is not ready for review. Also this patch is based off my other patches, so it might not apply to your tree.
Thanks for looking into this. On 2017/06/09 17:10:52, hub wrote: > I couldn't find an easy way to do the "require()" in elemHideEmulation. Which > mean that for now this is not ready for review. Do you mean for the unit tests, or when integrating into Chrome, or something else? (For Chrome we define require in buildtools/templates/modules.js.tmpl and we plan to use that for our content scripts too so it will be available.) > Also this patch is based off my other patches, so it might not apply to your > tree. No problem, could you link to the other codereviews which this depends on in this review's description?
Sorry for the spam, I meant to publish these inline comments at the same time... https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... lib/content/elemHideEmulation.js:18: /* eslint-env browser */ Why is this necessary? https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... lib/content/elemHideEmulation.js:457: if (typeof exports != "undefined") Don't we expect exports to always exist like for the other modules?
On 2017/06/12 12:49:38, kzar wrote: > Thanks for looking into this. > > On 2017/06/09 17:10:52, hub wrote: > > I couldn't find an easy way to do the "require()" in elemHideEmulation. Which > > mean that for now this is not ready for review. > > Do you mean for the unit tests, or when integrating into Chrome, or something > else? (For Chrome we define require in buildtools/templates/modules.js.tmpl and > we plan to use that for our content scripts too so it will be available.) For the unit test. I have things working in the chrome addon, in a separate patch. > > Also this patch is based off my other patches, so it might not apply to your > > tree. > > No problem, could you link to the other codereviews which this depends on in > this review's description? Done.
https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... lib/content/elemHideEmulation.js:18: /* eslint-env browser */ On 2017/06/12 12:50:13, kzar wrote: > Why is this necessary? The mistake here is that I should have the proper .eslintrc instead. Will fix that. We definitely need the browser env. https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... lib/content/elemHideEmulation.js:457: if (typeof exports != "undefined") On 2017/06/12 12:50:13, kzar wrote: > Don't we expect exports to always exist like for the other modules? Sadly no, we can't expect that yet. -in the tests, this is not loaded using require() at least until we fix this. -common.js does it for the same reason.
On 2017/06/12 13:10:16, hub wrote: > On 2017/06/12 12:49:38, kzar wrote: > > Thanks for looking into this. > > > > On 2017/06/09 17:10:52, hub wrote: > > > I couldn't find an easy way to do the "require()" in elemHideEmulation. > Which > > > mean that for now this is not ready for review. > > > > Do you mean for the unit tests, or when integrating into Chrome, or something > > else? (For Chrome we define require in buildtools/templates/modules.js.tmpl > and > > we plan to use that for our content scripts too so it will be available.) > > For the unit test. I have things working in the chrome addon, in a separate > patch. I see, so I guess we're going to have to add an implementation of `require` into adblockpluscore/test/_common.js to fix that, probably something similar to the implementation in buildtools/templates/modules.js.tmpl. I guess then as well the `if (typeof exports != "undefined")` check can be removed from adblockpluscore/lib/common.js. (I can't remember how all the glue that holds the tests together works, so there might be more to change as well.)
https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... lib/content/elemHideEmulation.js:18: /* eslint-env browser */ On 2017/06/12 13:26:33, hub wrote: > On 2017/06/12 12:50:13, kzar wrote: > > Why is this necessary? > > The mistake here is that I should have the proper .eslintrc instead. Will fix > that. > > We definitely need the browser env. Sorry, but I still don't understand why it's necessary? https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... lib/content/elemHideEmulation.js:457: if (typeof exports != "undefined") On 2017/06/12 13:26:34, hub wrote: > On 2017/06/12 12:50:13, kzar wrote: > > Don't we expect exports to always exist like for the other modules? > > Sadly no, we can't expect that yet. > > -in the tests, this is not loaded using require() at least until we fix this. > -common.js does it for the same reason. Ah right, well I think we need to fix that at the same time as we make this change. See my other reply.
https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... lib/content/elemHideEmulation.js:18: /* eslint-env browser */ On 2017/06/12 13:35:12, kzar wrote: > On 2017/06/12 13:26:33, hub wrote: > > On 2017/06/12 12:50:13, kzar wrote: > > > Why is this necessary? > > > > The mistake here is that I should have the proper .eslintrc instead. Will fix > > that. > > > > We definitely need the browser env. > > Sorry, but I still don't understand why it's necessary? This change is gone in the current version of the patch I use the .eslintrc.json that was in chrome/content It is necessary as we run in the browser environment where the `document` and `console` globals are defined. This is just carrying over from the existing code. https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... lib/content/elemHideEmulation.js:457: if (typeof exports != "undefined") On 2017/06/12 13:35:12, kzar wrote: > On 2017/06/12 13:26:34, hub wrote: > > On 2017/06/12 12:50:13, kzar wrote: > > > Don't we expect exports to always exist like for the other modules? > > > > Sadly no, we can't expect that yet. > > > > -in the tests, this is not loaded using require() at least until we fix this. > > -common.js does it for the same reason. > > Ah right, well I think we need to fix that at the same time as we make this > change. See my other reply. agreed.
https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... lib/content/elemHideEmulation.js:18: /* eslint-env browser */ On 2017/06/12 14:03:14, hub wrote: > On 2017/06/12 13:35:12, kzar wrote: > > On 2017/06/12 13:26:33, hub wrote: > > > On 2017/06/12 12:50:13, kzar wrote: > > > > Why is this necessary? > > > > > > The mistake here is that I should have the proper .eslintrc instead. Will > fix > > > that. > > > > > > We definitely need the browser env. > > > > Sorry, but I still don't understand why it's necessary? > > This change is gone in the current version of the patch I use the .eslintrc.json > that was in chrome/content > > It is necessary as we run in the browser environment where the `document` and > `console` globals are defined. This is just carrying over from the existing > code. Right now I understand, previously this file was ignored by ESLint but it has been renamed. I guess the old name should be removed from .eslintignore now. Also I seem to remember Wladimir disagreeing that the browser env should be added globally, but I can't remember why. I'll ask in IRC. https://codereview.adblockplus.org/29460576/diff/29463567/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29460576/diff/29463567/lib/content/elemHid... lib/content/elemHideEmulation.js:154: console.error(new SyntaxError("Failed parsing ABP " + These changes seem unrelated? (Same below.)
https://codereview.adblockplus.org/29460576/diff/29463567/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29460576/diff/29463567/lib/content/elemHid... lib/content/elemHideEmulation.js:154: console.error(new SyntaxError("Failed parsing ABP " + On 2017/06/12 14:12:44, kzar wrote: > These changes seem unrelated? (Same below.) you are looking at the interdiff.... it is not in the actual patch.
https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... lib/content/elemHideEmulation.js:18: /* eslint-env browser */ On 2017/06/12 14:12:44, kzar wrote: > On 2017/06/12 14:03:14, hub wrote: > > On 2017/06/12 13:35:12, kzar wrote: > > > On 2017/06/12 13:26:33, hub wrote: > > > > On 2017/06/12 12:50:13, kzar wrote: > > > > > Why is this necessary? > > > > > > > > The mistake here is that I should have the proper .eslintrc instead. Will > > fix > > > > that. > > > > > > > > We definitely need the browser env. > > > > > > Sorry, but I still don't understand why it's necessary? > > > > This change is gone in the current version of the patch I use the > .eslintrc.json > > that was in chrome/content > > > > It is necessary as we run in the browser environment where the `document` and > > `console` globals are defined. This is just carrying over from the existing > > code. > > Right now I understand, previously this file was ignored by ESLint but it has > been renamed. I guess the old name should be removed from .eslintignore now. > > Also I seem to remember Wladimir disagreeing that the browser env should be > added globally, but I can't remember why. I'll ask in IRC. This is now totally gone in the current patch since the ancestor patches actually address the problem (an actual bug)
https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... lib/content/elemHideEmulation.js:457: if (typeof exports != "undefined") On 2017/06/12 14:03:13, hub wrote: > On 2017/06/12 13:35:12, kzar wrote: > > On 2017/06/12 13:26:34, hub wrote: > > > On 2017/06/12 12:50:13, kzar wrote: > > > > Don't we expect exports to always exist like for the other modules? > > > > > > Sadly no, we can't expect that yet. > > > > > > -in the tests, this is not loaded using require() at least until we fix > this. > > > -common.js does it for the same reason. > > > > Ah right, well I think we need to fix that at the same time as we make this > > change. See my other reply. > > agreed. After reading through this review again I think removing this check and the similar one from common.js is what still needs doing. Could you remove those, fixing the related stuff you mentioned above?
Current patch is based on master. Test fail because now that we are using require we need to change the test harness to generate the content script like we do in the plugins and elsewhere. https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29460576/diff/29460577/lib/content/elemHid... lib/content/elemHideEmulation.js:457: if (typeof exports != "undefined") On 2017/07/07 13:42:20, kzar wrote: > On 2017/06/12 14:03:13, hub wrote: > > On 2017/06/12 13:35:12, kzar wrote: > > > On 2017/06/12 13:26:34, hub wrote: > > > > On 2017/06/12 12:50:13, kzar wrote: > > > > > Don't we expect exports to always exist like for the other modules? > > > > > > > > Sadly no, we can't expect that yet. > > > > > > > > -in the tests, this is not loaded using require() at least until we fix > > this. > > > > -common.js does it for the same reason. > > > > > > Ah right, well I think we need to fix that at the same time as we make this > > > change. See my other reply. > > > > agreed. > > After reading through this review again I think removing this check and the > similar one from common.js is what still needs doing. Could you remove those, > fixing the related stuff you mentioned above? Acknowledged.
Fairly straightforward change but this cannot land as long as tests aren't working. https://codereview.adblockplus.org/29460576/diff/29511631/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29460576/diff/29511631/lib/content/elemHid... lib/content/elemHideEmulation.js:517: exports.splitSelector = splitSelector; This shouldn't export splitSelector. If we need this functionality elsewhere, it should go into common.js or something like that.
https://codereview.adblockplus.org/29460576/diff/29511631/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29460576/diff/29511631/lib/content/elemHid... lib/content/elemHideEmulation.js:517: exports.splitSelector = splitSelector; On 2017/08/16 09:55:53, Wladimir Palant wrote: > This shouldn't export splitSelector. If we need this functionality elsewhere, it > should go into common.js or something like that. Acknowledged, OK I'll move that over while I'm at it.
Closing as it is now part of https://codereview.adblockplus.org/29517687/ 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
