Add support for json config files #899
Conversation
| function linkDependency(blob, key, value) { | ||
| let regex; | ||
|
|
||
| if (Number.isNaN(key)) { |
There was a problem hiding this comment.
When the value is an array the key values are numeric so jsonRegExValue doesn't work.
|
In addition to the ones you already added (👌) there’s also ESLint and any tools that also support adding their config to |
|
As @fregante already mentioned, ESLint might be interesting to cover as well, although from my experience many projects are using the Btw. I maintain a module called eslint-rule-docs which was intended to become part of OctoLinker as well at some point, but I fall into the trap of trying to cover all possible eslint config files. |
|
Just pushed some more changes that adds package.json support for ava, babel, eslint, stylelint, and xo. The issue with eslint is it seems like most of their references don't use the full package name so we're not able to link them. This probably needs its own plugin so we can handle looking those things up differently. |
|
This is looking great! |
| '$.stylelintConfig.extends': linkDependency, | ||
| '$.xo.extends': linkDependency, | ||
| '$.xo.parser': linkDependency, | ||
| }); |
There was a problem hiding this comment.
I wonder if it makes a difference performance wise to have dedicated plugins for each config, but most likely it's just micro optimisation. I'll look into it to get a better understanding if this is worth changing or not.
There was a problem hiding this comment.
I checked a few different config files and this code run on average in less than 5ms 😄
|
Looks amazing @xt0rted. I don't think we need to add E2E tests for all of them but maybe for the most common once like for babel, eslint and config within a package.json file |
|
@stefanbuck I was adding tests in for this and trying to get the rest of the areas implemented but noticed an issue in how the links are being handled. If the matched text exists more than once in the file, such as
This is the file I was testing against https://github.com/jassi1124/reaction-testing/blob/843f14aec572c3de4551b8e180698495ccd19458/package.json I'm not really sure what I can do to prevent this. |
|
Looks like a bug, which even exists for single dependencies. In the old implementation of |
|
🤦 I had two versions of OctoLinker running (local and the live version from the store) and therefor OctoLinker run twice. However, I'll look how we can fix it here |
|
Was the screenshot taken from your regular chrome instance or during E2E tests? I'm asking because it seems to work for me |
|
It was with my local dev instance from |
|
Mhhh is it still behaving like that? I run |
|
I have changes that weren't pushed up yet because I'm still playing with it. Give this branch a try. It's still a work in progress but that's the code that was triggering the issue for me. |
|
Strange, it's still behaving as expected.
|
|
Sorry it was my fault. I assumed |
|
but now it's too late for me 😉 I'll continue the hunt tomorrow |
|
I've been using a split terminal in vscode, one has |
|
I pushed a fix onto this branch c6dd225 which is fixing the issue by checking whether or not the node or parent is already wrapped. I need to think about how to test this best. |
|
I pushed a test case to ensure links are not wrapped twice. This covers c6dd225 |
How is that related? |
|
I'll finish this up next week |
By the availability of my time 😉 I have to juggle kids, my day job and way too many side projects which is quite difficult during these crazy times. |
|
I hope to see this merged eventually |
|
There is also |
Checklist:
This is a quick attempt at adding support for the json config format used by linting tools (they usually support js, json, and yaml but this is just for json right now). @fregante this was your request, do you know of any other json config files we could add to the list? I was looking at eslint but their format doesn't really work with this.
Closes #552
babel
stylelint
tslint