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

Add RecordProperty AST type #6

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JLHwung
Copy link
Collaborator

@JLHwung JLHwung commented Jun 9, 2020

View Rendered Text

This PR proposes a new node type RecordProperty which shares the same interface with ObjectProperty. This type is only valid in RecordExpression.

The new node type also paves the way for proper AST support on the stage-1 Deep Path Properties in Record Literals.

cc @rickbutton

@rickbutton
Copy link

My only concern is that the deep path properties proposal is also investigating usage with objects, although the proposal explainer hasn't been updated to reflect that. There was significant support for usage with objects in TC39, so splitting out a RecordProperty might not prove useful if ObjectProperty needs to support the additional syntax as well.

Deep Path Properties is also very early, and the syntax has a decent chance of changing. I would be hesitant to recommend additional work on your side with the expectation that deep path properties lands as designed.

@JLHwung
Copy link
Collaborator Author

JLHwung commented Jun 10, 2020

Given that it has not been decided whether DPP (Deep Path Properties) should be supported for object, I am good with holding off this proposal and delaying parser support on DPP.

I will leave this proposal open for community feedbacks. However, if decision is not made in the next 6 months, this proposal should be closed with respect to the RFC process.

Related: tc39/proposal-deep-path-properties-for-record#11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants