-
Notifications
You must be signed in to change notification settings - Fork 365
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
feat: global config sepc #1725
feat: global config sepc #1725
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
1dcbc69
to
6372217
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @JGAntunes and thank you for sharing your concerns.
The more I think about it the more I tend to agree with you.
I think we should default to using the old config and not remove it (at least as an initial step).
The live tunnel is one issue and the Large Media credential helper is another.
The latter is installed as an oclif
plugin:
https://docs.netlify.com/large-media/requirements-and-limitations/#requirements
so it will keep using the legacy path anyway.
We're in the process of migrating it so probably better to wait with removing the legacy path.
cliId: uuidv4(), | ||
} | ||
|
||
const getGlobalConfig = async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[sand] Not sure there is big benefit for it, but would it make sense to use lodash.once
to resolve the config only a single time.
For example in utils/command.js
the function will be invoked twice (once for the token and once for the whole config).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! Good one, I actually tested it out locally and saw some specifc commands could actually trigger a call to globalConfig
multiple times, I wanted to mention that but just forgot 🤦 I think some kind of memoisation like lodash.once
would definitely be a good thing here 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small suggestion: memoize-one
is also an option. It does the same thing but the code looks much clearer. Lodash code always looks so verbose :/
lodash.once()
:
var FUNC_ERROR_TEXT = 'Expected a function';
var INFINITY = 1 / 0,
MAX_INTEGER = 1.7976931348623157e+308,
NAN = 0 / 0;
var symbolTag = '[object Symbol]';
var reTrim = /^\s+|\s+$/g;
var reIsBadHex = /^[-+]0x[0-9a-f]+$/i;
var reIsBinary = /^0b[01]+$/i;
var reIsOctal = /^0o[0-7]+$/i;
var freeParseInt = parseInt;
var objectProto = Object.prototype;
var objectToString = objectProto.toString;
function before(n, func) {
var result;
if (typeof func != 'function') {
throw new TypeError(FUNC_ERROR_TEXT);
}
n = toInteger(n);
return function() {
if (--n > 0) {
result = func.apply(this, arguments);
}
if (n <= 1) {
func = undefined;
}
return result;
};
}
function once(func) {
return before(2, func);
}
function isObject(value) {
var type = typeof value;
return !!value && (type == 'object' || type == 'function');
}
function isObjectLike(value) {
return !!value && typeof value == 'object';
}
function isSymbol(value) {
return typeof value == 'symbol' ||
(isObjectLike(value) && objectToString.call(value) == symbolTag);
}
function toFinite(value) {
if (!value) {
return value === 0 ? value : 0;
}
value = toNumber(value);
if (value === INFINITY || value === -INFINITY) {
var sign = (value < 0 ? -1 : 1);
return sign * MAX_INTEGER;
}
return value === value ? value : 0;
}
function toInteger(value) {
var result = toFinite(value),
remainder = result % 1;
return result === result ? (remainder ? result - remainder : result) : 0;
}
function toNumber(value) {
if (typeof value == 'number') {
return value;
}
if (isSymbol(value)) {
return NAN;
}
if (isObject(value)) {
var other = typeof value.valueOf == 'function' ? value.valueOf() : value;
value = isObject(other) ? (other + '') : other;
}
if (typeof value != 'string') {
return value === 0 ? value : +value;
}
value = value.replace(reTrim, '');
var isBinary = reIsBinary.test(value);
return (isBinary || reIsOctal.test(value))
? freeParseInt(value.slice(2), isBinary ? 2 : 8)
: (reIsBadHex.test(value) ? NAN : +value);
}
module.exports = once;
memoize-one
:
function areInputsEqual(newInputs, lastInputs) {
if (newInputs.length !== lastInputs.length) {
return false;
}
for (var i = 0; i < newInputs.length; i++) {
if (newInputs[i] !== lastInputs[i]) {
return false;
}
}
return true;
}
function memoizeOne(resultFn, isEqual) {
if (isEqual === void 0) { isEqual = areInputsEqual; }
var lastThis;
var lastArgs = [];
var lastResult;
var calledOnce = false;
function memoized() {
var newArgs = [];
for (var _i = 0; _i < arguments.length; _i++) {
newArgs[_i] = arguments[_i];
}
if (calledOnce && lastThis === this && isEqual(newArgs, lastArgs)) {
return lastResult;
}
lastResult = resultFn.apply(this, newArgs);
calledOnce = true;
lastThis = this;
lastArgs = newArgs;
return lastResult;
}
return memoized;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @ehmicky! So I was actually looking at lodash.once
and was considering alternatives, however seems like we already have lodash as a dependency
- https://github.com/netlify/cli/blob/master/package.json#L132 - taking that into account would it be worth bringing another dependency? Might be something worth considering if we plan on decoupling lodash
from the project (or at least shift to depending only on the specific lodash modules we need).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
Either works, your call 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given we're keeping the "legacy" config path for now, would it be worth it creating an issue just to track this "tech debt"? CC @erezrokah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍
About adding an issue for future dead code removal: this would be a good idea.
Quick question: was there any way to use this.netlify.globalConfig
instead of calling getGlobalConfig()
in the telemetry and hooks code? This would remove the need to memoize. Feel free to discard this comment if not relevant, I was just curious :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @ehmicky. I guess it would be possible and it would probably make a lot of sense for the track/identify
telemetry functions to receive the required config via its parameters instead of requiring it. For the hooks however, I'm still digging through oclifs arch but seems like they don't have access to the base command instance - https://oclif.io/docs/hooks#lifecycle-events 😕. We also have this static getToken
function that depends on the global-config - https://github.com/netlify/cli/blob/master/src/utils/command.js#L313 - but seems like it's only being called on the deploy test - https://github.com/netlify/cli/blob/master/tests/command.deploy.test.js#L8 (?)
Tbh I think it makes sense to avoid this memoisation 🤔 maybe eventually remove it. Just not 100% of what would be the impact of doing it right now, given the global-config seems to be tied to a couple of different places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could move forward with memoisation
and open another issue to refactor the code to use this.netlify.globalConfig
.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! 👍
Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
8adff6f
to
3e87c3d
Compare
- Summary
Addresses #1530 (picking up on the discussion in there) and #526. The idea is to
cli
's global config to one place only (OS based) which for Linux means following the XDG base directory spec. We use the legacy directory config and use it as a default basis for the config init, once that is done we delete it.A couple of notes:
debug
logs to thegetGlobalConfig
stating we've found a legacy config and deleted it successfully? 🤔~/.netlify
directory is forliveTunnel
- https://github.com/netlify/cli/blob/master/src/utils/live-tunnel.js#L48 - specifically to hold thelive-tunnel
binary. Given we've changed thegetPathInHome
implementation this would mean that the binary would now live in the new config directory and would need to be downloaded again. However, we're not removing thelive-tunnel
bin or the whole~/.netlify
dir, so I'm wondering if it's better to (at least for now) just use the legacy path?- Test plan
- Description for the changelog
Chaging the global config path specification to a per OS standard.
- A picture of a cute animal (not mandatory but encouraged)
My very own cat soundcard!