Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jsstacktrace: add language #2418

Merged
merged 23 commits into from Jun 22, 2020

Conversation

sbrl
Copy link
Contributor

@sbrl sbrl commented Jun 9, 2020

I was getting really frustrated this afternoon with not being able to read long Javascript stack traces easily, so I implemented a simple grammar for Prism :-)

@RunDevelopment
Copy link
Member

RunDevelopment commented Jun 9, 2020

Seems interesting. JS stack traces are 100% implementation-defined, so this might not be 100% perfect but why not?

Before we can merge this:

  1. Please clean up the file. There are a lot of commented-out code snippets.
  2. This whole thing get's easier if you wrap every stack frame in a token and highlight its parts. This is easier and you don't have to worry about accidentally highlighting parts of the error message. (See Java stack trace)
  3. Please only use single quotes.
  4. Please separate words with dashes (-) in token names. (notmycode not-my-code).
  5. Please add an example page.

Also, maybe add support for deno as well? Right now it's very centered on node.

Examples stack traces from deno
Error: Some error
    at throwsA (<unknown>:1:23)
    at <unknown>:1:13
    at evaluate ($deno$/repl.ts:64:34)
    at Object.replLoop ($deno$/repl.ts:153:13)
Uncaught NotFound: No such file or directory (os error 2)
    at DenoError (deno/js/errors.ts:22:5)
    at maybeError (deno/js/errors.ts:41:12)
    at handleAsyncMsgFromRust (deno/js/dispatch.ts:27:17)

If you need help in any way, feel free to ask. The Java stack trace implementation should be a good example.

@sbrl
Copy link
Contributor Author

sbrl commented Jun 11, 2020

Thanks for the feedback! I'll review those comments and make some improvements.

@sbrl
Copy link
Contributor Author

sbrl commented Jun 11, 2020

Done, @RunDevelopment - all suggestions applied, if you could take another look?

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks a lot better!

I have a few suggestions. These should also fix most (if not all) of the highlighting errors in the examples page (it's really good that you included so many and so diverse examples!).

Also, there is an s missing in the name of the examples file.
Also also, it would be nice to highlight ( and ) as punctuation.

components/prism-jsstacktrace.js Outdated Show resolved Hide resolved
components/prism-jsstacktrace.js Outdated Show resolved Hide resolved
components/prism-jsstacktrace.js Outdated Show resolved Hide resolved
components/prism-jsstacktrace.js Outdated Show resolved Hide resolved
components/prism-jsstacktrace.js Outdated Show resolved Hide resolved
components/prism-jsstacktrace.js Outdated Show resolved Hide resolved
components/prism-jsstacktrace.js Outdated Show resolved Hide resolved
@sbrl
Copy link
Contributor Author

sbrl commented Jun 15, 2020

Ah, thanks for all those comments @RunDevelopment! I've made some additional changes. Some notes:

  • I've used your new patterns, but I made some tweaks:
    • < and > are needed in functions because a function can be anonymous - e.g. foo.<anonymous>
    • In your modified filename pattern, you accidentally dropped support for absolute paths on Windows. While I don't have a Windows machine myself (nor would I recommend it to anyone else), I think that supporting it is still a good idea.

Many great suggestions, thanks again!

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbrl Looking good!

Just a few minor things and then we can merge this.
(I think this is going to be the last review round.)

components/prism-jsstacktrace.js Outdated Show resolved Hide resolved
components/prism-jsstacktrace.js Outdated Show resolved Hide resolved
components/prism-jsstacktrace.js Outdated Show resolved Hide resolved
components.json Outdated Show resolved Hide resolved
@sbrl
Copy link
Contributor Author

sbrl commented Jun 16, 2020

Applied those changes, @RunDevelopment. Is this good to go now?

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for your work!

You have to rebuild to make the build pass and then we can merge this.

@sbrl
Copy link
Contributor Author

sbrl commented Jun 21, 2020

Phew! Glad to finally have this approved. Thanks for the help in the review process!

I've rebuilt it and pushed a new commit .

@RunDevelopment RunDevelopment merged commit ae0327b into PrismJS:master Jun 22, 2020
@RunDevelopment
Copy link
Member

Thank you for contributing @sbrl!

quentinvernot pushed a commit to TankerHQ/prismjs that referenced this pull request Sep 11, 2020
This language highlights JavaScript stack traces generated by commonly used JS engines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants