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: 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!

@nzakas nzakas merged commit be918b6 into main Apr 17, 2024
19 checks passed
@nzakas nzakas deleted the undefined-configs branch April 17, 2024 17:52
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.

None yet

3 participants