no-cli-ads icon indicating copy to clipboard operation
no-cli-ads copied to clipboard

Other version based on directly patching npm

Open mkg20001 opened this issue 4 years ago • 13 comments

I've made https://github.com/mkg20001/npm-adblock

It's basically the same, except it works by directly patching npm to only run postinstall hooks if they aren't blacklisted by the criteria outlined in https://github.com/mkg20001/npm-adblock/blob/master/index.js

That one should work on windows (should, but might not) It's a bit better since it is always enabled after installation and can even re-patch itself after npm self-updates (But updates via the package manager break it)

What do you think?

mkg20001 avatar Aug 26 '19 14:08 mkg20001

What if I want to run the postinstall scripts for the other modules that were installed? Can it blacklist a single postinstall script but run the others?

kethinov avatar Aug 26 '19 16:08 kethinov

It only blacklists the postinstall for modules that were explicitly added to the blacklist in https://github.com/mkg20001/npm-adblock/blob/master/index.js It matches either by module name or by strings in the command. for example when the hook command starts with "opencollective-postinstall" it gets blacklisted regardless of the module name. Should it ever break a module, because it's post install does more than display ads, then other workarrounds will be implemented to get just the ads blocked.

mkg20001 avatar Aug 26 '19 16:08 mkg20001

The blacklist currently is written by me as js code (and updated with the app), but if it were to grow bigger I might also serve a json file with patterns and let the client download it, similar to current adblockers.

mkg20001 avatar Aug 26 '19 16:08 mkg20001

@mkg20001 I like your approach a lot better actually. The code is also a lot more straight-forward.

adrianmcli avatar Aug 26 '19 16:08 adrianmcli

Assuming I'm reading all this correctly, it looks like there are some pros and cons.

Pros:

  • Cleaner code and probably scales more easily and easier to maintain (simple blacklist is easier than the tailoring required in my hacky approach).
  • Doesn't require running a persistent monitoring process.

Cons:

  • Alters npm itself, which is more invasive than a passive monitoring process.
  • Probably(?) doesn't allow partial execution of a postinstall script. For instance, I might want to run husky's postinstall script, but hide the donation beg.

kethinov avatar Aug 26 '19 17:08 kethinov

Probably(?) doesn't allow partial execution of a postinstall script

The postinstall can be modified in the filterHook function

Alters npm itself, which is more invasive than a passive monitoring process.

Yes and no. My Projects folder is huge. When I'd use your tool... it would hit some limit. That's why I made my version.

The second reason why I made the other tool, is because some modules have the ads as a node -e "console.log(...)" postinstall command. Since npm only writes the package.json to fs and doesn't read it again during the same installation, it can't be blocked via fs.

For example parcel does this with their ad.

mkg20001 avatar Aug 26 '19 17:08 mkg20001

Ah yeah, that's a good point. If you can modify the postinstall script using this method but still allow it to run, then the only distinction of relevance is whether a monitoring process feels ickier to you or modding npm itself does, which is somewhat subjective.

kethinov avatar Aug 26 '19 17:08 kethinov

then the only distinction of relevance is whether a monitoring process feels ickier to you or modding npm itself does, which is somewhat subjective.

Would be curious to see how something like this parcel ad can be blocked.

Checking /proc/PID/cmdline and killing it would be an option. But that way we can't get sure we kill it at the right time.

mkg20001 avatar Aug 26 '19 17:08 mkg20001

My monitoring process approach could catch it. Would need to alter watcher.js to scan for parcel being installed, then once it is alter the contents of its package.json at that line.

kethinov avatar Aug 26 '19 17:08 kethinov

Would need to alter watcher.js to scan for parcel being installed, then once it is alter the contents of its package.json at that line.

Does npm really re-read the package.json during the same install? I doubt that

mkg20001 avatar Aug 26 '19 17:08 mkg20001

Good question. This would have to be tested.

kethinov avatar Aug 26 '19 18:08 kethinov

Filed an issue for that. Also crosslinked your repo from this one's README.

kethinov avatar Aug 27 '19 13:08 kethinov

@kethinov I'm still curious to see if npm reloads the package.json. I looked through the npm code, and it only seems to write it if I'm not mistaken.

The only occurrence for readPackageJSON in install is lib/install.js:603 where it reads only the top package.json

  readPackageJson(path.join(this.where, 'package.json'), log, false, (err, data) => {
    if (err) return cb()
    this.currentTree = createNode({
      isTop: true,
      package: data,
      path: this.where
    })
    doOneAction('preinstall', this.where, this.currentTree, log.newGroup('preinstall:.'), cb)
  })

mkg20001 avatar Aug 27 '19 14:08 mkg20001