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

Prevent using prototype properties as units #56

Merged
merged 2 commits into from
Jan 24, 2025

Conversation

goto-bus-stop
Copy link
Contributor

If you try to use "1arguments", the code attempts to access parse.arguments, which throws an error in strict mode. If you try to use "2call", parse returns NaN, because it tries to use the parse.call function value in the calculation.

With this change, prototype properties are not considered when resolving units.

units = parse[units.replace(/s$/, '')]
} else {
units = null
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@goto-bus-stop do you think this would be better?
units = !Function.prototype[units] && (parse[units] || parse[units.replace(/s$/, '')])
seems one less check and shorter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then argumentss would trigger it (Function.prototype.argumentss does not exist, but then it does parse['argumentss'] || parse['arguments']). Also, accessing the arguments property on the prototype also throws an error:

~ > node -e 'Function.prototype.arguments'
[eval]:1
Function.prototype.arguments
                   ^

TypeError: 'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them
    at [eval]:1:20
    at runScriptInThisContext (node:internal/vm:209:10)
    at node:internal/process/execution:459:12
    at [eval]-wrapper:6:24
    at runScriptInContext (node:internal/process/execution:457:60)
    at evalFunction (node:internal/process/execution:285:30)
    at evalTypeScript (node:internal/process/execution:297:3)
    at node:internal/main/eval_string:71:3

Node.js v23.6.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is to say, i favour the straightforward, "correct" but longer way :)

Copy link
Collaborator

@dy dy Jan 24, 2025

Choose a reason for hiding this comment

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

@goto-bus-stop as a second thought, I wonder if parse.unit = {...} would be a better option here, instead of storing units on parse function directly.

@dy dy merged commit c99127b into jkroso:master Jan 24, 2025
@dy dy mentioned this pull request Jan 24, 2025
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

2 participants