Skip to content
This repository has been archived by the owner on Jun 10, 2024. It is now read-only.

fix: Throw friendly message for non-object configs #136

Merged
merged 2 commits into from
Apr 17, 2024
Merged

Conversation

nzakas
Copy link
Contributor

@nzakas nzakas commented Apr 15, 2024

Adds explicit validation and clear error messages for:

  1. undefined configs
  2. null configs
  3. non-object configs

All of these checks happen during normalization.

refs eslint/eslint#18259

@nzakas
Copy link
Contributor Author

nzakas commented Apr 15, 2024

cc @mdjermanovic @fasttime

Comment on lines -54 to 67
super(`Config ${name}: ${source.message}`, { cause: source });
constructor(name, index, { cause, message }) {


const finalMessage = message || cause.message;

super(`Config ${name}: ${finalMessage}`, { cause });

// copy over custom properties that aren't represented
for (const key of Object.keys(source)) {
if (!(key in this)) {
this[key] = source[key];
if (cause) {
for (const key of Object.keys(cause)) {
if (!(key in this)) {
this[key] = cause[key];
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously, when an Error object was passed as source, this code copied the properties from that Error object and set that Error object as the cause of the ConfigError. Now it's copying the properties from Error.cause and setting Error.cause as the cause of the ConfigError, so the properties of the Error object are lost and the Error object won't be in the cause chain. I'm not yet sure if this is causing some observable differences in functionalities at the moment, but it might be safer to revert to the previous behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the function signature but forgot to update the call sites to use the new signature. It should work the same now. (I guess this is where type checking would have been helpful!)

Copy link
Contributor

@fasttime fasttime left a comment

Choose a reason for hiding this comment

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

LGTM. Since the new changes are causing some tests in ESLint to fail, it would be good to release this in v0.13.x, so we can fix the tests while upgrading the dependency.

Copy link
Contributor

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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.

3 participants