ignore arbitrary @import in comment #156
Conversation
|
|
||
| options.relativeTo = options.relativeTo || options.root; | ||
| options._baseRelativeTo = options._baseRelativeTo || options.relativeTo; | ||
| options.visited = options.visited || []; | ||
|
|
||
| comments = data.match(/(\/\*(?!\*\/)[\s\S]*?\*\/)|(\/\/[^\n]*)/g); |
There was a problem hiding this comment.
That's a nice regexp! We probably should lazy load this stuff in the loop below as if there are no @import statements then scanning whole data for comments is pointless.
|
Good stuff, thanks for spending time on implementing it! We need to check how it affects performance in real world scenarios too as the current bench task ( |
|
It's now fully lazy, and will only search for comments if any What would you like to do regarding performance test? Should we add a test file with imports in it? It would make sense to measure that in the benchmark anyway I guess :) |
| correctedEndIndex = endIndex + lastEndIndex; | ||
|
|
||
| if (correctedEndIndex < idx) | ||
| return scanner(idx); |
There was a problem hiding this comment.
Spend a little while trying to understand the rewrite, but so far no luck with this line - what is the point re-running scanner with the same idx as the original one? This is probably to mitigate intermediate comments which does not have an @import inside them.
It could probably lead to stack overflow exception unless there is something tricky in it :-)
|
Good stuff with making it lazy! Most We currently use |
|
That's a really sharp catch! I accidentally forgot to move the data chopping statement during refactoring. Now Sounds good about benchmarking. Let me know if there's something I can do about it. |
|
Looks great now! If you don't mind I'll add the @import to benchmarking tomorrow morning (already have an idea how to reorganize it) so you can rebase and see how it performs. Then we'll merge your code to master. Do you consider it as a patch to 1.1 or should we ship it with upcoming 2.0 only?
|
|
Great, thanks! I'd say this is more of a patch, since it's not a feature, but rather a fix, so a patch to 1.1 seems fit :) |
|
I agree. Once merged we will push 1.1.7 out. Stay tuned for updates.
|
|
@altschuler please take a look at ef54fc0 - it now uses I applied this patch to your HEAD, then put a random |
|
Yes, I can reproduce this, seems a bit odd. I'll take a deeper look later today. |
|
Cool! It may be helpful to trim |
|
I've fixed this now and updated my branch (rebased from master). The bug was somewhat subtle but I find it rather odd that it worked at all. The code is a bit more verbose now, but should be easier to understand and more manageable :) I tested it with many different imports in different places. I also couldn't measure any performance impact, event with loads of "fake" imports. |
|
@altschuler looks great! Two minor things and we are done:
|
|
Agreed :) all done |
|
Sorry, one more thing - can you prepend commit's message with 👍 |
Ignore arbitrary @import in comments.
|
@altschuler 1.1.7 is out with your patch. Many thanks for the important contribution! |
|
Glad I could help! |
@import used in comments without "url (..." causes "Broken @import declaration" exceptions to be thrown.
It's probably not the most efficient solution as it saves all comment positions in memory, but it solved the problem.