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: Make properties of 'e' enumerable #101

Merged
merged 2 commits into from
Jul 17, 2024
Merged

fix: Make properties of 'e' enumerable #101

merged 2 commits into from
Jul 17, 2024

Conversation

SimonAlling
Copy link
Owner

@SimonAlling SimonAlling commented May 26, 2024

I ran into a confusing problem in SimonAlling/better-sweclockers#264. Consider an operation with this implementation:

export default (e: {
    foo: HTMLElement,
    bar: HTMLElement,
    baz: HTMLElement,
}) => {
    for (const element of Object.values(e)) {
        console.log(element);
    }
};

One would expect that all three elements would be logged, but this operation actually does nothing at all. The reason is that the properties of e aren't enumerable, so Object.values(e) is an empty array.

While it's easy to come up with pathological examples of operations that will be broken by this change, it should be backward compatible for all practical intents and purposes.

💡 git show --color-words=.

@SimonAlling SimonAlling merged commit 3c7aee1 into master Jul 17, 2024
5 checks passed
@SimonAlling SimonAlling deleted the enumerable branch July 17, 2024 13:06
@SimonAlling SimonAlling mentioned this pull request Jul 17, 2024
SimonAlling added a commit that referenced this pull request Jul 17, 2024
This release contains a fix for the issue that operation dependencies
(the properties of `e`) surprisingly aren't enumerable (#101).
SimonAlling added a commit to SimonAlling/better-sweclockers that referenced this pull request Jul 18, 2024
Version 3.0.1 includes SimonAlling/userscripter#101, which will let us DRY up some code slightly.
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