Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
style(eslint): apply specified code style changes #138
style(eslint): apply specified code style changes #138
Changes from 1 commit
54574c8
ebe562d
c33049e
c34389b
607e6c0
24ced7d
3209b2a
ffc58ba
6da9d15
96e08c3
8e963d1
fcb48a3
5dce043
79f5d80
e66a051
5c7205c
74f22dc
d13b925
4545791
1b80afe
a14acd2
881139f
c2f6013
c077e4f
934c84e
b137820
4fc93ee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
That might work; need to double-check, but just so that we're on the same page, this is what we are referred to when we say that it returns a "map-like object":
https://github.com/google/closure-compiler/wiki/A-word-about-the-type-Object#map-like-objects
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.
see https://mdn.io/relect-get
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.
I believe the
Reflect.get
static method would also be able to access any inherited properties (e.g., from the baseObject
superclass — all the properties from the constructor would be included), which i think we'd like to avoid.For security purposes, i believe only having access to all enumerable own properties would be safer and more efficient iiuc.
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.
I have started to collect some research on the topic at https://github.com/openinf/util-object/wiki/Type-Annotation-for-JavaScript-Objects, which we'll be incorporating as part of our style guide. Once I have settled on our preference, i'll be sure to make the update here.
/cc @septs as this would be another guideline to be added to our JS authoring guide (and enforced via our standard) ^^
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.
Object
->object
Number
->number
String
->string
BigInt
->bigint
Symbol
->symbol
Boolean
->boolean
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.
For this one, that would seem alright, but it would be very loose as it would simply mean any non-nullish value. Wonder what you think about the TypeScript
Record
utility type?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.
I don't like that it is called the same as
Record
from the TC39 proposal, seeing as how they have different semantics iiuc. The TypeScriptRecord
utility type isn't an immutable object afaik, so mixing that in our codebase is a bit annoying imo.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.
see https://github.com/tc39/proposal-function-memo
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.
Yeah, i knew about that proposal, but am not yet sure how it relates to this function. Is the difference that this is Object memo and that would be Function memo?
/cc @js-choi