Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #3253 - make User-Agent configurable #3255

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

m-idler
Copy link

@m-idler m-idler commented Oct 31, 2023

Adds a configuration option to overwrite the default User-Agent header that is send at least by the calendar and news module. Allows other modules to use the individual user agent as well.

The configuration accepts either a string or a function:

var config =
	{
		...
		userAgent: 'Mozilla/5.0 (My User Agent)',
		...
	}

or

var config =
	{
		...
		userAgent: () => 'Mozilla/5.0 (My User Agent)',
		...
	}

@khassel
Copy link
Collaborator

khassel commented Oct 31, 2023

Thank you for this PR!

First thing, the test fails are unrelated to your changes, we had problems with one unit test which are now fixed on develop, so can you please merge develop into your branch (or rebase on develop), thanks.

I'm not sure if we need the new getUserAgent() function because we store defaults in js/defaults.js, so AFAIK would be the normal way to add the entry

userAgent: `Mozilla/5.0 (Node.js ${Number(process.version.match(/^v(\d+\.\d+)/)[1])}) MagicMirror/${global.version}`,

in this file and then use this property with config.userAgent in the modules and the tests.

But before refactoring this may @rejas can review and tell us what he prefers ...

@m-idler m-idler force-pushed the feature/#3253-user-agent-configuration branch from 9c3e6e3 to 49c8353 Compare November 1, 2023 19:25
@m-idler
Copy link
Author

m-idler commented Nov 1, 2023

I'm not sure if we need the new getUserAgent() function because we store defaults in js/defaults.js, so AFAIK would be the normal way to add the entry

userAgent: `Mozilla/5.0 (Node.js ${Number(process.version.match(/^v(\d+\.\d+)/)[1])}) MagicMirror/${global.version}`,

in this file and then use this property with config.userAgent in the modules and the tests.

In the beginning I thought about that as well, but it felt a little bit odd to have all those variables in a configuration string by default. Also I wasn't sure when those variables are available and in case someone wants to use other runtime variables in here, that might lead to issues - that's why I added the possibility to use it with a callback as well.

I also thought about, wether it would make sense to add a parameter (e.g. the module name, url, ...) to the method, so that it could be possible to use different user agents based upon those parameters.

Based on those thoughts i decided to wrap it in a utility method to allow easier, non-breaking, changes in the future. But if you prefer to consume the config.userAgent directly, I'm also fine with it 😉

@khassel
Copy link
Collaborator

khassel commented Nov 1, 2023

and I forgot something yesterday because config.userAgent doesn't work, the module only gets his own config section, not the global one (so the default userAgent had to be set in every module).

So I'm fine with merging this.

@khassel khassel requested a review from rejas November 1, 2023 20:12
@sdetweil
Copy link
Collaborator

sdetweil commented Nov 1, 2023

since when does config. not work?

you have to pass it from browser side to node helper,

@khassel
Copy link
Collaborator

khassel commented Nov 1, 2023

may I'm wrong but AFAIK you can't get e.g. config.language in the node_helper.js of the module (not without special coding)

@sdetweil
Copy link
Collaborator

sdetweil commented Nov 1, 2023

yes, but 'special' is tiny..

from my MMM-Config

  notificationReceived: function (notification, payload, sender) {
    // once everybody is loaded up
    if (notification === "ALL_MODULES_STARTED") {
      // send our config to our node_helper
      
      // copy  global config info to module config  
      this.config.address = config.address;     
      this.config.port = config.port;
      this.config.whiteList = config.ipWhitelist;
      
      this.sendSocketNotification("CONFIG", this.config);   // this sends to node_helper
    }
  },

@khassel
Copy link
Collaborator

khassel commented Nov 1, 2023

so for using config.userAgent instead of the implemented function in this PR we had to do this.config.userAgent= config.userAgent; in the 2 modules.

I'm fine with both solutions.

@sdetweil
Copy link
Collaborator

sdetweil commented Nov 1, 2023

and then in node_helper to use it, its payload.config.userAgent (or whatever variable payload is stored in)
from the sockerReceivedNotifications.

and you can get it ALL with

this.config.global_config = config

we have no cross module protection on the config data

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants