Skip to content

Conversation

alexanderchr
Copy link

Still not sure how to open the editor at the correct line/position. At least in atom you can do atom editor filename:line but it might break other editors.

One option would to let the user supply a templated string to exec instead - like atom #{file}:#{row}.

if (error.error) {
body = 'Error: ' + body + error.error.toString();
line = error.error.loc.line;
column = error.error.loc.column;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is loc always available?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Turbo87
Copy link
Owner

Turbo87 commented Apr 13, 2016

I like the idea of that templated string, although it might cause troubles when using exec(command, [arg1, arg2]) instead of a plain string

@alexanderchr
Copy link
Author

Yeah - if we were to use a templated string I think we'd have to drop using the second argument of exec.

@alexanderchr alexanderchr force-pushed the master branch 3 times, most recently from 98aaa09 to df3f9d7 Compare April 13, 2016 14:08
@Turbo87
Copy link
Owner

Turbo87 commented Apr 13, 2016

Yeah - if we were to use a templated string I think we'd have to drop using the second argument of exec.

not necessarily. we could allow editor: ['atom', '#{file}:#{row}'], use the first item in the array as the command and the others as arguments and replace the tokens in each of the arguments.

@alexanderchr alexanderchr force-pushed the master branch 2 times, most recently from d99678b to b04959f Compare April 13, 2016 14:12
@alexanderchr
Copy link
Author

Do you have any reason for wanting that syntax over a simple string? I believe that it'd make a simpler api.

Also checked the doc for child process. If we want to send the arguments in an array we will have to use spawn instead of exec. For our purposes they are essentially the same - the only difference is in how they return output.

@Turbo87
Copy link
Owner

Turbo87 commented Apr 13, 2016

@alexanderchr I am worried about escaping properly if e.g. the filepath contains spaces or any other kinds of special characters (&, ...)

@alexanderchr alexanderchr force-pushed the master branch 3 times, most recently from 4467913 to efc1d5d Compare April 13, 2016 19:40
@alexanderchr
Copy link
Author

@Turbo87 True. Didn't think about that.

I've implemented the templated strings i spoke of earlier - please see the changed commits.

@Turbo87
Copy link
Owner

Turbo87 commented Apr 13, 2016

@alexanderchr cool, thanks! I'll try to test this tomorrow.

@Turbo87
Copy link
Owner

Turbo87 commented Apr 14, 2016

@alexanderchr I've just tested this and in my current dependency setup the location info is not available at error.loc but at error.lineNumber and error.column directly.

@alexanderchr
Copy link
Author

Interesting. What loaders are you using?

@Turbo87
Copy link
Owner

Turbo87 commented Apr 15, 2016

@alexanderchr I just used the project in the example folder of this repo

@alexanderchr
Copy link
Author

Strange, I get { [SyntaxError: Unexpected token (1:14)] pos: 14, loc: Position { line: 1, column: 14 }, raisedAt: 25 } when running webpack in the example folder.

I'm running node v5.6.0, what are you using?

@Turbo87
Copy link
Owner

Turbo87 commented Apr 20, 2016

@alexanderchr node v5.8.0

it's been a while since I ran npm install in that project though so I might have some older dependencies in there. if that is the issue then some dependency apparently didn't care much about semver though.

@alexanderchr
Copy link
Author

I upgraded to 5.8.0, still getting my errors in loc. Did npm install change anything for you?

@Gvozd
Copy link
Collaborator

Gvozd commented Dec 7, 2020

@alexanderchr
It seems to work for me
Within a week I will update the branch to the current master
I think it will be possible to release this feature in experimental status, and collect feedback from users

@Gvozd
Copy link
Collaborator

Gvozd commented Dec 8, 2020

@alexanderchr
your code is rebased to the current master
welcome to new iteration - #63 😄

@Gvozd Gvozd closed this Dec 8, 2020
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