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
Filter default properties in json middlewares instead of filter fn #6
base: syncback-merge
Are you sure you want to change the base?
Filter default properties in json middlewares instead of filter fn #6
Changes from 1 commit
f635cdc
6b9e515
579f55c
b54eee1
6eb04bf
b57f473
e335caa
ea4af65
09c4e2c
7cdda8a
ca488ac
0d8a36d
583666a
cae0c9b
bcfdc0c
a854e8a
59d780c
3dd6856
a4f10c6
b2fd639
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.
Should this warning also provide the path of the file where the instance is defined?
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.
We can actually include
SharedString
values in JSON now because of rojo-rbx/rbx-dom#414. I just forgot to change it.We can probably just remove this warning altogether.
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.
Do you mind if we tackle that in a future PR? Making that change effects several tests and I'd rather not clutter this PR with them
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, we can handle that in a different PR. A bit out of scope for this one either way.
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.
If we allow rbx-dom's rbx_reflector to write defaults for SharedString properties (which is now possible after rojo-rbx/rbx-dom#414), then we won't have to alter any test data
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 think there's a compelling reason to not have rbx-dom write SharedString defaults now. Seems like a good change.