Add column range to babel-code-frame #5646

SimenB · 2017-04-18T18:20:01Z

Q	A
Patch: Bug Fix?	no
Major: Breaking Change?	no
Minor: New Feature?	yes
Deprecations?	no
Spec Compliancy?	no
Tests Added/Pass?	yes
Fixed Tickets	#5064
License	MIT
Doc PR	No
Dependency Changes	Yes

Inspired by flow errors, this allows us to highlight a whole word or phrase, not just a single character.

I can make this out to the 7.0 branch instead, and not make the argument optional? As in, same as
colNumber, but handle null and don't try to support both 4 and 5 args.

If accepted, I can update babel's own usage of babel-code-frame to use this new option. I didn't do it yet in case you don't want this 😄

mention-bot · 2017-04-18T18:20:03Z

@SimenB, thanks for your PR! By analyzing the history of the files in this pull request, we identified @lydell, @hzoo and @DrewML to be potential reviewers.

SimenB · 2017-04-18T18:20:44Z

packages/babel-code-frame/src/index.js

@@ -1,6 +1,7 @@
 import jsTokens, { matchToToken } from "js-tokens";
 import esutils from "esutils";
 import Chalk from "chalk";
+import repeat from "lodash/repeat";


This seemed to be the way lodash was used

SimenB · 2017-04-18T18:24:26Z

packages/babel-code-frame/src/index.js

  opts: Object = {},
 ): string {
  colNumber = Math.max(colNumber, 0);

+  if (typeof colRange === "object") {


This is the part that can be dropped if a breaking change is acceptable

codecov · 2017-04-18T18:32:17Z

Codecov Report

Merging #5646 into 7.0 will decrease coverage by 0.02%.
The diff coverage is 88.67%.

@@            Coverage Diff            @@
##              7.0   #5646      +/-   ##
=========================================
- Coverage   84.63%   84.6%   -0.03%     
=========================================
  Files         282     282              
  Lines        9857    9899      +42     
  Branches     2766    2777      +11     
=========================================
+ Hits         8342    8375      +33     
- Misses        995    1002       +7     
- Partials      520     522       +2

Impacted Files	Coverage Δ
packages/babel-code-frame/src/index.js	`94.17% <88.67%> (-0.91%)`	⬇️
packages/babel-helper-call-delegate/src/index.js	`64% <0%> (-4%)`	⬇️
packages/babel-traverse/src/path/context.js	`84.48% <0%> (-1.73%)`	⬇️
packages/babel-traverse/src/path/modification.js	`73.78% <0%> (-0.98%)`	⬇️
...bel-plugin-transform-es2015-classes/src/vanilla.js	`90.12% <0%> (-0.86%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6646707...bc8ef0d. Read the comment docs.

existentialism · 2017-04-18T18:43:26Z

@SimenB thanks for the PR!

There was some discussion about specifying a range in code-frame and breaking vs not in #5064 (also a WIP PR: #5515)

SimenB · 2017-04-18T18:48:39Z

I failed searching apparently...
If we don't want to break backwards this PR can be considered ready for review? Unless we want to accept an object as column: {start: number, end: number} in addition to a single number instead of a new argument (which would be cleaner)?

If the approach in #5515 is preferred this can be closed, but that PR seems stalled. Maybe not though 😄

It might have stalled because of lack of feedback for all I know.

mohsen1 · 2017-04-20T00:12:56Z

Hi @SimenB,

I can't find time to learn Flow and finish that PR. I'll close it in favor of this. I'm in favor of the new API I proposed in #5064.

This need to cover

single position
single line with no column
single line range (from and to positions)
multi line range from and to specific position
multi line range with no column data

SimenB · 2017-04-20T06:57:23Z

@mohsen1 Sure, I can do that 😄
@existentialism Target master or 7.0?

existentialism · 2017-04-20T22:25:53Z

Let's keep it targeting 7.0

SimenB · 2017-04-22T15:20:53Z

Rebased on 7.0, and now taking in { start: { column, line }, end: { column, line } }. Any comments so far? (ignore readme)

@mohsen1 How did you imagine multiple lines would look?

SimenB · 2017-04-22T15:26:57Z

packages/babel-code-frame/test/index.js

@@ -30,19 +30,6 @@ describe("babel-code-frame", function () {
    ].join("\n"));
  });

-  it("optional column number", function () {


this test is duplicated right above it (which meant mocha didn't run it)

SimenB · 2017-04-22T17:48:30Z

I added some stuff for multiple lines. Not sure how useful it is, but it's there at least 😄

@existentialism, ready for review. Not really happy about all the +1s and -1s, but I'm not sure how to avoid them... The tests pass, so that's something 😄

EDIT:got away from most of them, so OK now

mohsen1 · 2017-04-22T18:51:10Z

Based on how TSLint test runner marks ranges this is what I would expect:

single position

  1 | function foo() {
> 2 |   return 1;
    |          ^
  3 | }

single line with no column

  1 | function foo() {
> 2 |   return 1;
  3 | }

single line range (from and to positions)

  1 | function foo() {
> 2 |   return 1;
    |   ~~~~~~~~       
  3 | }

multi line range from and to specific position

> 1 | function foo() {
    |                ~
> 2 |   return 1;
    | ~~~~~~~~~~~
> 3 | }
    | ~

multi line range with no column data

> 1 | function foo() {
> 2 |   return 1;
> 3 | }

SimenB · 2017-04-22T19:18:18Z

Thanks! Then I think the current implementation was correct, except for multi lines without column data. Fixed that now though. Can you take a look at the tests and see if you agree everything is covered?

EDIT: Should probably check out tslint's implementation, figure out why they wrote their own instead of reusing babel-code-frame.

EDIT2: They already use this module: https://github.com/palantir/tslint/blob/master/src/formatters/codeFrameFormatter.ts
What reporter do you use for tslint?

hzoo · 2017-05-01T21:51:17Z

Sorry for the wait!

Can we add a section to the readme about how to upgrade to the new format?

SimenB · 2017-05-02T01:41:03Z

@hzoo No probs, glad it wasn't forgotten 😄

Readme updated (and I rebased)

SimenB · 2017-05-02T02:08:27Z

packages/babel-code-frame/src/index.js

+  if (!deprecationWarningShown) {
+    deprecationWarningShown = true;
+
+    console.warn(new Error(


Not sure how I feel about this

SimenB · 2017-05-14T10:13:45Z

@hzoo Sorry for the noise, but is there anything that needs changing? Would hate for you to circle back and want changes whenever you're ready to merge this, much rather it was good to go.

SimenB · 2017-05-14T10:14:40Z

packages/babel-code-frame/src/index.js

+  colNumber: ?number,
+  opts: Object = {},
+): string {
+  if (!deprecationWarningShown) {


Do we want to print a deprecation, or just let it be?

hzoo

lgtm, sorry been a long time for this PR 😅

hzoo · 2017-05-19T21:27:42Z

I'm ok without the deprecation too, or we can make a simple codemod for this too

existentialism · 2017-05-19T21:30:01Z

We can turn on flow for these files later :)

existentialism · 2017-05-31T19:23:18Z

@SimenB thanks again!

hzoo · 2017-06-01T18:45:20Z

Added in v7.0.0-alpha.12 (Example PR to update #5808) - dono if we want the deprecation message or not still if it's going to cause other tools to error but maybe it's ok

SimenB · 2017-06-01T19:49:49Z

I'm all for just removing the old API and throwing :)

SimenB · 2017-06-02T20:02:14Z

@hzoo This is a breaking change for require anyways, as the default export is now only on default. Should we just replace the replace the original export?

SimenB · 2017-06-02T20:06:01Z

Can fix off by one at the same time?

hzoo · 2017-06-07T19:49:49Z

also from @rwjblue on slack

would be great to be able to get the offenders info into deprecations like that
would be great to use callsites and report the calling filename, line, col info

SimenB · 2017-06-08T06:03:27Z

Not sure I understand that one. Just do --trace-deprecation (possibly --trace-warnings) when running to get at least a stacktrace.

Or is a very pretty stakctrace wanted? If so, why? And if you really want it, could you provide a sample of the wanted output?

hzoo · 2017-06-08T14:16:16Z

We just need to know the file that the import/use of babel-code-frame is at - we could even use babel-code-frame to do it 😛 .

Just saying the current output is not helpful because it only tells you to change it but not where.

Just check our travis tests: https://travis-ci.org/babel/babel/jobs/240419278

SimenB · 2017-06-08T14:21:54Z

Yeah, but if you start node with --trace-warnings then you will get the full stack trace

rwjblue · 2017-06-08T14:36:29Z

I think in general we should strive to make the default console output have enough information so that folks can track this down. Having the filename, column, and line info would do that.

I doubt that most users know about process.emitWarning or --trace-warnings...

hzoo · 2017-06-08T14:51:29Z

I definetely didn't know about it 😛

SimenB · 2017-06-08T15:02:18Z

It's the official way to deprecate stuff in node. Maybe we could print a hint about it? If you use node 4 it's a normal stacktrace, though.

hzoo · 2017-06-08T15:04:46Z

Sure just thinking about the end-user - they are going to see this in their output and wonder what to do. It might not even be their own code so we should update the main consumers to be updated beforehand (prettier which you did, eslint, css-loader, create-react-app, etc)

SimenB · 2017-06-08T15:06:13Z

I can go through an send a PR to them. Should we try to fix the off by one first, though?

And should we just remove the bridge and throw? That way we don't have to deal with the trace at all

hzoo · 2017-06-08T15:08:30Z

Since this is 7.x I guess we could just throw if it makes it easier, shouldn't be hard to do together? Clearly it didn't work since we didn't fix all the places in babel itself 😛 so unlikely the case for others too

SimenB · 2017-06-08T15:11:59Z

We already create an Error, easier to just throw it than it is to check for process.emitWarning etc 🙂

Clearly it didn't work since we didn't fix all the places in babel itself

True story!

SimenB commented Apr 18, 2017

View reviewed changes

SimenB force-pushed the code-frame-range branch from 8abe3dd to 4e0d2a7 Compare April 18, 2017 18:23

SimenB commented Apr 18, 2017

View reviewed changes

xtuc added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Apr 21, 2017

SimenB force-pushed the code-frame-range branch from 4e0d2a7 to 07ef1e4 Compare April 22, 2017 13:22

SimenB changed the base branch from master to 7.0 April 22, 2017 13:23

SimenB force-pushed the code-frame-range branch from 07ef1e4 to f769b98 Compare April 22, 2017 14:56

SimenB commented Apr 22, 2017

View reviewed changes

SimenB force-pushed the code-frame-range branch 2 times, most recently from 9c54350 to 9a30d3a Compare April 22, 2017 17:47

SimenB force-pushed the code-frame-range branch 3 times, most recently from 213bc49 to 9e1635e Compare April 22, 2017 17:57

SimenB force-pushed the code-frame-range branch from 9e1635e to 635ec5d Compare April 22, 2017 19:17

SimenB force-pushed the code-frame-range branch from 635ec5d to 84407b5 Compare April 23, 2017 07:38

SimenB force-pushed the code-frame-range branch from 84407b5 to 4193e3e Compare May 2, 2017 01:40

SimenB commented May 2, 2017

View reviewed changes

SimenB force-pushed the code-frame-range branch from e8fecd6 to bc8ef0d Compare May 6, 2017 22:39

SimenB commented May 14, 2017

View reviewed changes

hzoo approved these changes May 19, 2017

View reviewed changes

existentialism approved these changes May 19, 2017

View reviewed changes

hzoo merged commit 63b7137 into babel:7.0 May 31, 2017

SimenB deleted the code-frame-range branch May 31, 2017 19:26

hzoo mentioned this pull request May 31, 2017

Allow specifying a range in babel-code-frame #5064

Closed

SimenB mentioned this pull request Jun 2, 2017

Don't call deprecated code frame export #5815

Merged

SimenB mentioned this pull request Jun 2, 2017

Upgrade babel code frame prettier/prettier#1902

Merged

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019

Add column range to babel-code-frame #5646

Add column range to babel-code-frame #5646

Conversation

SimenB commented Apr 18, 2017 • edited Loading

mention-bot commented Apr 18, 2017

SimenB Apr 18, 2017

Choose a reason for hiding this comment

SimenB Apr 18, 2017

Choose a reason for hiding this comment

codecov bot commented Apr 18, 2017 • edited Loading

Codecov Report

existentialism commented Apr 18, 2017

SimenB commented Apr 18, 2017 • edited Loading

mohsen1 commented Apr 20, 2017 • edited Loading

SimenB commented Apr 20, 2017

existentialism commented Apr 20, 2017

SimenB commented Apr 22, 2017 • edited Loading

SimenB Apr 22, 2017

Choose a reason for hiding this comment

SimenB commented Apr 22, 2017 • edited Loading

mohsen1 commented Apr 22, 2017 • edited Loading

single position

single line with no column

single line range (from and to positions)

multi line range from and to specific position

multi line range with no column data

SimenB commented Apr 22, 2017 • edited Loading

hzoo commented May 1, 2017 • edited Loading

SimenB commented May 2, 2017

SimenB May 2, 2017

Choose a reason for hiding this comment

SimenB commented May 14, 2017

SimenB May 14, 2017

Choose a reason for hiding this comment

hzoo left a comment

Choose a reason for hiding this comment

hzoo commented May 19, 2017

existentialism commented May 19, 2017

existentialism commented May 31, 2017

hzoo commented Jun 1, 2017

SimenB commented Jun 1, 2017

SimenB commented Jun 2, 2017

SimenB commented Jun 2, 2017

hzoo commented Jun 7, 2017

SimenB commented Jun 8, 2017 • edited Loading

hzoo commented Jun 8, 2017

SimenB commented Jun 8, 2017

rwjblue commented Jun 8, 2017 • edited Loading

hzoo commented Jun 8, 2017

SimenB commented Jun 8, 2017

hzoo commented Jun 8, 2017

SimenB commented Jun 8, 2017

hzoo commented Jun 8, 2017 • edited Loading

SimenB commented Jun 8, 2017

SimenB commented Apr 18, 2017 •

edited

Loading

codecov bot commented Apr 18, 2017 •

edited

Loading

SimenB commented Apr 18, 2017 •

edited

Loading

mohsen1 commented Apr 20, 2017 •

edited

Loading

SimenB commented Apr 22, 2017 •

edited

Loading

SimenB commented Apr 22, 2017 •

edited

Loading

mohsen1 commented Apr 22, 2017 •

edited

Loading

SimenB commented Apr 22, 2017 •

edited

Loading

hzoo commented May 1, 2017 •

edited

Loading

SimenB commented Jun 8, 2017 •

edited

Loading

rwjblue commented Jun 8, 2017 •

edited

Loading

hzoo commented Jun 8, 2017 •

edited

Loading