Change removal of sourcemap comment #3987
Conversation
|
|
yannayl
force-pushed
the
fix-module-parse-ast-remove-sourcemapping-comment
branch
from
8de4260
to
aebeefa
Compare
Codecov Report
@@ Coverage Diff @@
## master #3987 +/- ##
=======================================
Coverage 97.22% 97.22%
=======================================
Files 191 191
Lines 6731 6743 +12
Branches 1970 1969 -1
=======================================
+ Hits 6544 6556 +12
Misses 99 99
Partials 88 88
Continue to review full report at Codecov.
|
yannayl
force-pushed
the
fix-module-parse-ast-remove-sourcemapping-comment
branch
2 times, most recently
from
2b9fd3d
to
d613643
Compare
|
I think it's working well. Though I would love to have the logic of this fix revised. |
Do the removal without relying on the module parsing the code. This way even if a plugin uses the `parse` method in the PluginContext the comments are removed.
yannayl
force-pushed
the
fix-module-parse-ast-remove-sourcemapping-comment
branch
from
d613643
to
7386b1e
Compare
| } | ||
| } | ||
|
|
||
| return ret; |
There was a problem hiding this comment.
I think there might be some potential to speed this up: Allocating arrays and slices of strings for every top-level statement is not for free as I can tell you from experience.
If you would do this the other way around, it might be much more efficient:
- Run your (stateful) regular expression over the entire code (in a loop). This should be quite fast as it is not creating a lot of new data structures (in the current edition, you are running it over all the code that is part of a top-level statement, which is nearly the same amount of code, but split up in many steps)
- In this loop for each match, determine if it is in a top-level statement. This might be done by just looping over the statements, or quite fast by doing a binary search through the statement list. I can recommend this algorithm as it does not use recursion and does not allocate any data structures: https://flaviocopes.com/binary-search-javascript/
There was a problem hiding this comment.
in the current edition, you are running it over all the code that is part of a top-level statement
No, I am not. I am running over the spaces between top-level statements. Which shouldn't be very large - usually only newlines and comments.
Allocating arrays and slices of strings for every top-level statement is not for free
AFAIK slices of strings in V8 are pretty much for free if it's RO (SlicedString is just a pointer to the parent and offset), see this https://mrale.ph/blog/2016/11/23/making-less-dart-faster.html for more details. And these slices, as mentioned, are quite small anyway (and they live very shortly, which means they die in the allocator's young space and never re-allocated).
The ranges array can't be overwhelmingly big, all on all the number of top-level statements is something humans should be able to cope with. So it can't be in the millions...
Do you still think it would be better to implement the idea you suggested?
If so, I will but I am afraid it will hurt readability (which IMHO is more important).
There was a problem hiding this comment.
No, I am not. I am running over the spaces between top-level statements
Ah I see, I misread that.
The ranges array can't be overwhelmingly big
I would not underestimate this, if a file is not deeply nested it is not too far off from the order of the AST nodes (with a factor of maybe 10 between those)
But you could still get rid of the ranges entirely by inlining the regular expression check into the first loop? Just replace the push with the check?
Also, extract the code.slice out of the loop (even though we expect there to be only one).
Better readability and hopefully more performant
yannayl
force-pushed
the
fix-module-parse-ast-remove-sourcemapping-comment
branch
from
6811b0a
to
67d2ed9
Compare
Do the removal without relying on the module parsing the code. This way even if
a plugin uses the
parsemethod in the PluginContext the comments are removed.This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
#3982
Description
This PR moves the removal of sourceMappingURL comments away from code parsing. This means that even if a plugin parses the ast, the comment will still be removed.