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

Fix configuration with .screepsrc #9

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

Conversation

Hiestaa
Copy link

@Hiestaa Hiestaa commented May 10, 2020

I tried without success to configure this module using the .screepsrc method. I believe I am using Node v10, but I am not sure as I am running screeps server on windows through steam.

Digging through code, it appears that this line doesn't work as I would have expected:

Object.assign(opts.history, DEFAULTS, opts.history, ENV)

I would expect the above to pick values from ENV, then opts.history, then DEFAULT. And thus, with an empty ENV and values in opts.history, it should override the default.

However, that's not what happens. Consider this example:

> a = {x: 1, y: 2}
{ x: 1, y: 2 }
> b = {x: 10, y: 20}
{ x: 10, y: 20 }
> Object.assign(a, b, a)
{ x: 10, y: 20 }
> a
{ x: 10, y: 20 }

I believe what happens internally in Object.assign(a, b, a) is something along those lines:

  • Set a.x (first param) to b.x (second param), which set a.x to 10
  • Set a.x (first param) to a.x (second param) as the last param takes priority, but now a.x = 10 and thus we're just overriding a.x with the same value, a.x.
  • Same with a.y and b.y

It does not happen with the following code, because the object being set is brand new:

> a = Object.assign({}, a, b, a)
{ x: 1, y: 2 }
> a
{ x: 1, y: 2 }

I made the update in my local node_modules and the config in ./.screepsrc is honored.

If you're able to reproduce the problem on your end here is a fix for you! I think this might still be good to ship because that's the way MDN recommend to do merging:

Merging objects with same properties

const o1 = { a: 1, b: 1, c: 1 };
const o2 = { b: 2, c: 2 };
const o3 = { c: 3 };

const obj = Object.assign({}, o1, o2, o3);
console.log(obj); // { a: 1, b: 2, c: 3 }

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#Merging_objects_with_same_properties

Cannot use first param of `Object.assign()` as a mean to set the values in `opts.history`, as it set each field using values from the `DEFAULT` before setting them to the values of `opts.history` which have then already been lost to the defaults.
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.

1 participant