Skip to content
This repository was archived by the owner on Nov 6, 2023. It is now read-only.

Fix modules - #12622

Closed
ghost wants to merge 23 commits into
masterfrom
unknown repository
Closed

Fix modules#12622
ghost wants to merge 23 commits into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Sep 18, 2017

Copy link
Copy Markdown

@RReverser

RReverser commented Sep 18, 2017

Copy link
Copy Markdown
Contributor

Thanks for fixing! Although not sure all the duplicated wrappers are worth it TBH, especially given that this still doesn't fix rules.js being not directly require-able from Node, still works only in extension unless you do some global patching.

@ghost

ghost commented Sep 18, 2017

Copy link
Copy Markdown
Author

Just copy rules.js and put a background.js shim in the same folder.

@RReverser

Copy link
Copy Markdown
Contributor

No, I already explained in previous PR why it doesn't work that way... background.js uses Chrome-specific APIs even at top level, and it's not intended to be executed from Node.js. rules.js can and should be just a self-contained module instead of relying on any browser-specific bits.

@ghost

ghost commented Sep 18, 2017

Copy link
Copy Markdown
Author

@RReverser That's the shim. Use it instead of copying background.js.

'use strict'

module.exports = {
  enableMixedRulesets: false,
  domainBlacklist: {
    has () { return false }
  }
}

@RReverser

Copy link
Copy Markdown
Contributor

Which means the end script has to provide own shims just to require rewriter? Even for things like #12558 I would have to keep separate shim and preload it into global before requiring rules.js? I don't really see why this complication is needed when a much simpler solution can be done that wouldn't require any extra effort on consumption side...

@ghost

ghost commented Sep 18, 2017

Copy link
Copy Markdown
Author

Hmm, how?

@ghost

ghost commented Sep 18, 2017

Copy link
Copy Markdown
Author

I guess a copy of rules.js and the shim can be copied into a npm package and kept somewhere in the repo. Scripts may just use it.

@RReverser

Copy link
Copy Markdown
Contributor

Of course, anything can be shimmed, worked around, patched, hacked etc. on consumer side, but why not make it simple in the first place when it's as easy as moving two variables between files? All I want is to be able to launch Node.js, type require('./rules') and have it already working.

Hmm, how?

As wrote here: #12586 (comment)

Basically make rules.js (or, ideally, each script) independent, just providing APIs for options that can be modified externally.

@ghost

ghost commented Sep 18, 2017

Copy link
Copy Markdown
Author

Okay. This PR will fix two bugs then.

@cowlicks

Copy link
Copy Markdown
Contributor

Hey @koops76 @RReverser I'm working on fixing this, so please hold off for a bit until I can get it out.

@ghost

ghost commented Sep 19, 2017

Copy link
Copy Markdown
Author

@RReverser Added standalone rules.js support.

Comment thread chromium/rules.js Outdated
if (platform) {
default_state = false;
if (platform == "mixedcontent" && background.enableMixedRulesets) {
if (platform == "mixedcontent" && background.enableMixedRulesets.enable) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be without background.?

Comment thread chromium/background.js Outdated

// Allow users to enable `platform="mixedcontent"` rulesets
var enableMixedRulesets = false;
rules.enableMixedRulesets = false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed .enable?

Comment thread chromium/rules.js
if (platform) {
default_state = false;
if (platform == "mixedcontent" && background.enableMixedRulesets.enable) {
if (platform == "mixedcontent" && enableMixedRulesets.enable) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw why do you need this indirection and not just enableMixedRulesets being a boolean directly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work since it needs to be the same object.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be done with get() and set() methods instead.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you can just use exports.enableMixedRulesets. Then it will be same inside and outside.

@cowlicks

Copy link
Copy Markdown
Contributor

@koops76 would you mind marking this as WIP in the title so we know not to review it?

@ghost

ghost commented Sep 19, 2017

Copy link
Copy Markdown
Author

@cowlicks Wait until Travis will finish.

@cowlicks

Copy link
Copy Markdown
Contributor

@koops76 oops sorry. I meant to comment on your ES6 imports PR. Can you mark that is WIP so we know not to review it.

Also, I'm much more in favor of using #12556 to fix this.

@ghost

ghost commented Sep 19, 2017

Copy link
Copy Markdown
Author

@cowlicks Done.

@ghost

ghost commented Sep 19, 2017

Copy link
Copy Markdown
Author

@cowlicks It's closed, and if you fix all conflicts and merge it anyway, it will cause conflicts for this PR.

@cowlicks

Copy link
Copy Markdown
Contributor

Module fixes are being done in #12627 is this still needed?

@ghost ghost closed this Sep 19, 2017
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants