[WIP]glslify support #68
Conversation
|
It parses sourcemaps and shows the error line (almost) correctly.
|
| @@ -19,6 +19,9 @@ | |||
| "atom-linter": "^10.0.0", | |||
| "atom-message-panel": "^1.2.4", | |||
| "atom-package-deps": "^4.3.1", | |||
| "convert-source-map": "^1.5.0", | |||
| "glslify": "https://github.com/fand/glslify#sourcemaps", | |||
There was a problem hiding this comment.
I'm not sure I really want to use a branch on a fork here, that's just led to outdated and unmaintained code in every previous case.
There was a problem hiding this comment.
yea, this is just an experimental fork to get sourcemaps and disable minification.
I'm gonna replace it with glslify if it supports sourcemaps.
| @@ -183,6 +207,11 @@ export default { | |||
| default: false, | |||
| order: 2, | |||
| }, | |||
| useGlslify: { | |||
There was a problem hiding this comment.
This probably should have a description on here describing what this is actually doing.
lib/linter-glsl.js
Outdated
| const colStart = parseInt(match[2], 10); | ||
| const lineEnd = lineStart; | ||
| const colEnd = colStart; | ||
| let l = parseInt(match[3], 10); |
There was a problem hiding this comment.
Just use lineStart and colStart as the original code, there is no reason to switch to obscure names only to put them back to understandable ones right after.
lib/linter-glsl.js
Outdated
| @@ -294,6 +329,22 @@ export default { | |||
| args = ['-l']; | |||
| } | |||
| } | |||
| if (this.useGlslify) { | |||
| filesToValidate.forEach((f) => { | |||
| const baseDir = f.fullFilename.replace(/\/[^/]*$/, ''); | |||
There was a problem hiding this comment.
Is this just the equivalent of path.dirname?
There was a problem hiding this comment.
yep, replaced it with path.dirname 🙇
yarn.lock
Outdated
| @@ -0,0 +1,1663 @@ | |||
| # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
There was a problem hiding this comment.
I'm very much not a fan of yarn, having had it break too many projects. Please remove this file from the PR, thanks!
|
This is a very interesting idea, I'm not too sure about moving forward with it here though until support is more baked into the tools behind this. Thoughts @andystanton? |
|
@fand @Arcanemagus I think this is a fantastic idea. As you've already discussed, the sourcemap work needs to be merged in glslify first, but then we can definitely take a look at the best way to integrate it. I would embrace making |
It's still experimental.
Please give me some advice if you are interested 😸
glslify/glslify#95
Changes
Use glslifyto config