Website code and warnings

[enzoferey]

cc @NTillmann

Following up #2285, website shows the generated coded even when there are some warnings.

Lot of changes because prettier wasn't applied to the files, but I basically only checked for the error buffer containing only items with severity === 'Warning' and create the warning result type to behave the same than success one.

Next step would be to show the code AND warning messages. Either using the same display than error messages or creating a variation of it (changing text color to yellow for example). I can do it if you want.

Let me know if this is what you were looking for.

Have a nice day !

P.S: not sure if this PR was supposed to be into gh-pages or master. Let me know if that needs to be changed.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[enzoferey]

Fixed an unnoticed issue with heap graph. It doesn't seem to work anyways because the boxNetwork variable created from the data is never used (it never worked it seams). Plus when warnings happen but code is still generated (as it is the case with the snippet from the issue), no graph data is returned from the compiler (that's why I added an extra check against undefined).

There is a lot of discrepancy in code style along the repl.js and repl-worker.js as arrow functions are sometimes used, sometimes not; var keyword is used sometimes, others let is used instead...

I don't know how clean you want this code to be, but I could refactor it at the same time I add the warnings display.

Also, I noticed that error messages overflow the code result box and other CSS issues.

I think having a great playground plays an important part in getting people excited about the project, it should be very smooth imho.

Let me know what you think 👍

[NTillmann]

Thanks a lot for taking this on!

I made two inline change requests that should be made before we land this:

• Also allow informational diagnostics.
• No need for a 'warning' result type

Your PR would be easier to review without the formatting changes to clean up other parts of the code. So you could break up this PR into one with the functional changes, and then there could be another PR on top that only has formatting changes.

Taking a step back...

• Since we don't currently enforce formatting of the website files, these style issues will keep coming up. Even better than changing the formatting issues just now would be to also plug in some linter checks in Circle CI.
• What your change enables is showing the code when there are just warnings, but the downside is that warnings are no longer shown at all... Ideally, the UI would show both, while still nicely visually distinguishing the code and diagnostics messages.

[enzoferey]

Hi @NTillmann, thanks for the feedback.

I think the best is to work first on the logic and displaying messages next to code and later on discuss about formatting in a different PR as you suggested. I agree on the CI check.

I have updated the branch without any formatting change and I will work on the remaining task in the next days 👍

[enzoferey]

So... I had some time before the weekend ! 👍

@NTillmann, I have covered the requested changes and added support to display the messages and the code at the same time. Furthermore, every message has it owns node rather than just appending them in a single big one as it was done before. This allows showing multiple messages of different types and have proper styling on each of them !

I took heavy inspiration from the Google Chrome DevTools console for color palettes. Let's see how each case looks like:

Error

Warning

Errors

Warnings

Error and warning

My only concern is in the first error screenshot for example, we have a first line with the error code and basic info and just underneath the stack which might seem like a another error. This is because Prepack sends the errors split. There are several ways to fix it but I would need to know if every time Prepack sends an error without a code field means it's related to the last error (see below line):

https://github.com/facebook/prepack/blob/2457103e19b1de6b4347e2b522b67e63aee2e31a/website/js/repl.js#L132

I hope this was what you wanted, have a nice weekend ! 🚀

[hermanventer]

Prepack does not report a single issue with multiple error call backs. It does, however, report the same issue multiple times because the abstract interpreter may execute the same code block several times for various reasons.

Your UI changes are looking good, but I would prefer to see the error messages in the same pane as the source code since they pertain to the source code, not the generated code. I also wonder if there is a way to highlight the source line (preferably the source extent) to which an error refers.

[enzoferey]

Hi @hermanventer !

Do you mean having error messages at the bottom of the source pane and warnings at the bottom of the generated code pane ?

Regarding the highlight of the errors it should be possible as we have code line and position data from the errors. Would having the line with a red background and showing the error code on hover would be okay for you ? Or hovering on a message highlights the line it's refering to ? Both ? Do you have any idea of what would feel natural to you ?

[NTillmann]

Very nice! So much better than it used to be. @hermanventer has some good request on top...

Do you mean having error messages at the bottom of the source pane and warnings at the bottom of the generated code pane ?

All messages at the bottom of the source pane.

highlight of the errors

What we currently have is when you click on a message with an error location, then it highlights the corresponding source code line. That would be totally sufficient for me.

[enzoferey]

Okay !

So I have moved all messages to the bottom of the source pane. How it looks:

Error:

Warning:

Error and warning:

Regarding the source line code highlight, @hermanventer could you please confirm that the current functionality is enough ?

Have a nice weekend !

[NTillmann]

Very nice! This is almost perfect!

Only one final request: When there is some kind of fatal error, and Prepack produces no output, in your last version, it keeps saying // Compiling... on the right. The user might think that something is still working and not quite done, and wait... Can you simply remove that text in that case? I think even an empty output pane is better than a persistent // Compiling... text.

Many thanks!

[enzoferey]

Hi !

Wouldn't it be better to have a valuable feedback message like "// There was some error. Please check left pane" instead of a empty output ?

[hermanventer]

It would be better, yes. Perhaps "// Prepack is unable to produce output for this input. Please check the left pane for diagnostic information."

[enzoferey]

Done !

[NTillmann]

Many thanks for doing this, and going through all our incremental change requests!

[facebook-github-bot]

NTillmann is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[enzoferey]

Thank you guys for working on such a complex and game changer project. Let me know if you want me to help out on any other website related issues/improvements (we talked earlier about code formating and code quality).