Opened 14 months ago

Closed 14 months ago

Last modified 13 months ago

#6675 closed defect (fixed)

info module placeholder file included in AdBlock bundle by WebPack

Reported by: BrentM Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-3.2-for-Chrome-Opera-Firefox
Module: Platform Keywords: adblock, webpack
Cc: tlucas, kzar, sebastian Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29784585/

Description (last modified by kzar)

Environment

Windows 10 version 1803
Chrome 66.0.3359.139 (Official Build) (64-bit)
AdBlock devenv build from the adblock-next-gen/tree/update-ABP-Core branch.

How to reproduce

  1. Open the background console for the extension.

Complete a local dev build, and side load the extension.

Observed behaviour

The following JavaScript exception:

 Uncaught (in promise) Error: Attempt to change preference type
     at Object.set (prefs.js:229)
     at Object.set [as currentVersion] (prefs.js:328)
     at detectFirstRun (subscriptionInit.js:57)

Expected behaviour

No exceptions.

Notes

  • The "info-loader" rule only applies for the path adblock-next-gen/buildtools/info.js, but the info module is required with the relative path by the Adblock Plus code and that ends up resolving to adblock-next-gen/adblockpluschrome/buildtools/info.js.

Hints for testers

  • Check that element hiding and the options page still work on Firefox and Chrome.
  • Check that that the "Block element" button doesn't show up on Firefox for Android.
  • Check the first run page and updates page still shows up as expected.
  • Check the uninstall page still populates the fields like application and platform.

Change History (11)

comment:1 Changed 14 months ago by kzar

  • Cc tlucas kzar sebastian added
  • Component changed from Core to Automation
  • Description modified (diff)
  • Keywords adblock webpack added; buildtools removed
  • Owner set to kzar
  • Summary changed from BuildTools for AdBlock to info module placeholder file included in AdBlock bundle by WebPack

comment:2 Changed 14 months ago by kzar

  • Description modified (diff)

comment:3 Changed 14 months ago by kzar

One solution is to alter the "info-loader" rule so that it applies for both dummy file paths:

module: {
  rules: [{
    include: /buildtools\/info\.js$/,
    use: ["info-loader"]
  }]
},

Problem is that then results in two copies of the info module being bundled if we require it using both paths.

comment:4 Changed 14 months ago by kzar

I think the best plan is probably to avoid relative paths when requiring the info module, it's a special case.

comment:5 Changed 14 months ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:6 Changed 14 months ago by sebastian

  • Priority changed from Unknown to P2
  • Ready set

comment:7 Changed 14 months ago by abpbot

A commit referencing this issue has landed:
Issue 6675 - Don't use relative path for info module require

comment:8 Changed 14 months ago by kzar

  • Component changed from Automation to Platform
  • Description modified (diff)
  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next

comment:9 Changed 14 months ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed

Let me know if this is working for you now Brent.

comment:10 Changed 14 months ago by BrentM

Looks good. The initial / first run logic is now working. Thanks everyone.

comment:11 Changed 13 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

This looks fixed in AB and has caused no problems in ABP.

ABP 3.1.0.2069
Chrome 67 / 64 / 49 / Windows 7
Firefox 60 / 55 / 51 / Windows 7
Opera 52 / 45 / 38 / Windows 7

Note: See TracTickets for help on using tickets.