@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.

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

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.

@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)

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.

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

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

Let's keep it targeting 7.0

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?

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

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 | }

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?

Sorry for the wait!

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

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

Readme updated (and I rebased)

@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.

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

We can turn on flow for these files later :)

@SimenB thanks again!

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

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

@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?

Can fix off by one at the same time?

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

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?

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

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

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...

I definetely didn't know about it 😛

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.

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)

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

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

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!