NEW: Add support for parsers and the first parser. #720
Conversation
sarvaje
requested review from
alrra,
austinkelleher and
molant
as code owners
| } | ||
|
|
||
| private parseJavascript(fetchEnd: IFetchEnd) { | ||
| if (fetchEnd.response.mediaType !== 'text/javascript') { |
There was a problem hiding this comment.
What about application/javascript? I know a few issues were opened in sonarwhal discussing which media type is right, but here maybe both should be considered?
There was a problem hiding this comment.
The response.mediaType is calculated in sonar, and in case of a javascript, it will be always text/javascript
| if you can understand it. A good approach would be: | ||
|
|
||
| 1. check first the `mediaType` of the response (`fetchEndEvent#response#mediaType#`) | ||
| 2. use a schema to validate if `json` or `xml` or something similar |
There was a problem hiding this comment.
json,xml, or something similar
| event is emitted for all downloaded resources, you need to check first | ||
| if you can understand it. A good approach would be: | ||
|
|
||
| 1. check first the `mediaType` of the response (`fetchEndEvent#response#mediaType#`) |
There was a problem hiding this comment.
Should this be: fetchEndEvent.response.mediaType?
|
@alrra done! |
|
|
||
| The built-in `parser`s are: | ||
|
|
||
| * `javascript`: A `JavaScript` parser built on top of `ESLint` so rules for |
There was a problem hiding this comment.
We need to add more information on how a rule can subscribe to this events and create rules based on it.
There was a problem hiding this comment.
Do you have some rule in mind? Can it be addressed in another PR?
There was a problem hiding this comment.
Documentation on what's exposed via the parser and how to use it should be in this PR IMO.
There was a problem hiding this comment.
@molant I have added something else to the documentation.
|
|
||
| const scriptContentRegex: RegExp = /^<script[^>]*>([\s\S]*)<\/script>$/; | ||
| const defaultParserOptions = { | ||
| comment: true, |
There was a problem hiding this comment.
We should add a comment why we are using these options by default, and maybe a link?
| } | ||
|
|
||
|
|
||
| private isJavascript(element: IAsyncHTMLElement) { |
There was a problem hiding this comment.
Can you rename this to something such as: isJavaScriptType.
| this.emitScript(code, resource); | ||
| } | ||
|
|
||
| private isSrcPresent(element: IAsyncHTMLElement) { |
There was a problem hiding this comment.
Can you rename this to: hasSrcAttribute?
|
@alrra done! |
|
I have added the wizard to create new core/external parsers. |
sarvaje
force-pushed
the
parsers
branch
2 times, most recently
from
98d2f38
to
dbe8713
Compare
|
@sarvaje there are conflicts here. Can you fix them? Also there seems to be errors with AppVeyor? |
|
@molant I think the error with appVeyor is not related with this PR :s |
|
@molant conflics solved |
|
🎉 |
Pull request checklist
Make sure you:
For non-trivial changes, please make sure you also:
Short description of the change(s)