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

Jump to bottom

feat(react-jsx-source): add columnNumber property #11139

Merged
merged 2 commits into from Mar 16, 2020
Merged

feat(react-jsx-source): add columnNumber property #11139

merged 2 commits into from Mar 16, 2020

Conversation

motiz88
Copy link
Contributor

@motiz88 motiz88 commented Feb 13, 2020 •
edited

This will be useful to tools that consume the injected __source prop, allowing precise source locations to be displayed.

cc @rickhanlonii

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? 👍
Tests Added + Pass? 👍
Documentation PR Link babel/website#2178
Any Dependency Changes?
License MIT

@JLHwung JLHwung added area: jsx PR: New Feature 🚀 A type of pull request used for our changelog categories labels Feb 13, 2020
@nicolo-ribaudo
Copy link
Member

Should we call it columnNumber?

@motiz88
Copy link
Contributor Author

motiz88 commented Feb 13, 2020

Yeah, probably. We could make up a convention that ...Number is for 1-based values and thus leave it as column (or change to columnIndex) to signify that it's 0-based... But that's just me having been bitten by too many off-by-one bugs involving source locations :) I'll change it to whatever you prefer.

@motiz88
feat(react-jsx-source): add column property
6d5988d 
This will be useful to tools that consume the injected `__source` prop, allowing precise source locations to be displayed.
@motiz88
Copy link
Contributor Author

motiz88 commented Feb 15, 2020

Fixed tests and corrected the information in the PR description (I had a 👍 under Breaking Change, which this isn't)

motiz88 added a commit to motiz88/website that referenced this pull request Feb 15, 2020
@motiz88
react-jsx-source: add column property
806a8c6 
Docs PR to go with babel/babel#11139
@motiz88 motiz88 mentioned this pull request Feb 15, 2020
Merged
@kaicataldo
Copy link
Member

columnNumber makes sense to me.

@motiz88
Copy link
Contributor Author

motiz88 commented Feb 16, 2020

I'll change to columnNumber. Actually, how do you feel about making it 1-based as well?

@motiz88 motiz88 changed the title feat(react-jsx-source): add column property feat(react-jsx-source): add columnNumber property Feb 16, 2020
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Feb 17, 2020 •
edited

I slightly prefer 0-based column numbers, but it seems that other user-facing tools start counting from 1:

tool line start column start
VS Code 1 1
Chrome stack traces 1 1
@babel/parser 1 0
acorn 1 0
ESLint 1 1

(I'm ✔️ing for the implementation, not for the 1-based number, but I won't block this PR if it starts from 1 anyway)

@nicolo-ribaudo nicolo-ribaudo added this to the v7.9.0 milestone Feb 17, 2020
@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Feb 17, 2020
@nicolo-ribaudo
Copy link
Member

@motiz88 Out of curiosity, what tool would use this information?

@motiz88
Copy link
Contributor Author

motiz88 commented Feb 26, 2020

The specific thing we have in mind is showing better code frames for component errors in React Native.

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Mar 7, 2020
Merged
@nicolo-ribaudo nicolo-ribaudo merged commit 1971b7e into babel:master Mar 16, 2020
nicolo-ribaudo pushed a commit to babel/website that referenced this pull request Mar 17, 2020
@motiz88
react-jsx-source: add columnNumber property (#2178)
b3609a8 
* react-jsx-source: add column property

Docs PR to go with babel/babel#11139

* column -> columnNumber, make 1-based

Updated for babel/babel@9fb61dd
This was referenced Mar 22, 2020
Merged
Merged
Merged
Merged
Merged
This was referenced Apr 12, 2020
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Apr 20, 2020
Open
Open
Open
Open
Open
Open
This was referenced Apr 27, 2020
Closed
Open
Open
Merged
Open
Merged
Open
Closed
This was referenced May 5, 2020
Open
Open
Open
Open
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 16, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@existentialism existentialism existentialism approved these changes

@JLHwung JLHwung JLHwung approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

@kaicataldo kaicataldo kaicataldo approved these changes

Assignees
No one assigned
Labels
area: jsx outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release
Projects
None yet
Milestone
v7.9.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@motiz88 @nicolo-ribaudo @kaicataldo @existentialism @JLHwung