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

What Went Wrong exercise: Remove warning about live server code injection and line numbers #36541

Merged
merged 10 commits into from
Nov 1, 2024

Conversation

MaoShizhong
Copy link
Contributor

@MaoShizhong MaoShizhong commented Oct 28, 2024

Description

Motivation

In the "What went wrong?" exercise, there is a warning about line numbers in error messages potentially not matching source code line numbers due to code injection from extensions like live-server.

This is a result of the code's script tag being placed after the closing body tag, so live-server's injected code comes before it (as it injects right before the closing body tag).

By placing the script tag before the closing body tag, live-server injects code after it, preserving line numbers in the error messages, making the lesson much easier to follow for more people. The relevant warning can then be removed from the lesson. This warning and exercise has thrown a few people off in The Odin Project curriculum, and they are apprehensive about using tools like live-server.

This has been tested with VSCode's Live Server extension, VSCode's Live Preview extension and the live-server npm package.

Related issues and pull requests

Depends on: mdn/learning-area#775

Removed live-server injection warning as that will be fixed in a PR for the example code itself
To reflect fixes in linked lesson code file (some were already
inaccurate for the old code)
@MaoShizhong MaoShizhong requested a review from a team as a code owner October 28, 2024 17:01
@MaoShizhong MaoShizhong requested review from Josh-Cena and removed request for a team October 28, 2024 17:01
@github-actions github-actions bot added Content:Learn:JavaScript Learning area JavaScript docs size/s [PR only] 6-50 LoC changed labels Oct 28, 2024
wbamberg pushed a commit to mdn/learning-area that referenced this pull request Oct 30, 2024
## Because

In the ["What went wrong?"
exercise](https://developer.mozilla.org/en-US/docs/Learn/JavaScript/First_steps/What_went_wrong),
there is a warning about line numbers in error messages potentially not
matching source code line numbers due to code injection from extensions
like live-server.

This is a result of the code's script tag being placed *after* the
closing body tag, so live-server's injected code comes before it.

By placing the script tag *before* the closing body tag, live-server
injects code after it, preserving line numbers in the error messages,
making the lesson much easier to follow for more people. The relevant
warning can then be removed from the lesson.

This has been tested with [VSCode's Live Server
extension](https://marketplace.visualstudio.com/items?itemName=ritwickdey.LiveServer),
[VSCode's Live Preview
extension](https://marketplace.visualstudio.com/items?itemName=ms-vscode.live-server)
and the [live-server npm
package](https://www.npmjs.com/package/live-server).

## This PR

- Moves the closing body tag down *after* the script tag closes.
- Fixes inconsistent indentation across the file to only use 2 spaces
per level (rather than mixing spaces and tabs).
- Uses appropriate indentation for each block as necessary.

## Additional information

This PR depends on (mdn/content#36541) which
amends the line number references and screenshots in the lesson itself
to match what these code changes will produce.
@wbamberg
Copy link
Collaborator

@MaoShizhong , CI is complaining because it wants you to compress the screenshots in this PR. You should be able to do that by running:

yarn filecheck 'files/en-us/learn/javascript/first_steps/what_went_wrong/console-log-output.png' --save-compression
yarn filecheck 'files/en-us/learn/javascript/first_steps/what_went_wrong/not-a-function.png' --save-compression
yarn filecheck 'files/en-us/learn/javascript/first_steps/what_went_wrong/variable-is-null.png' --save-compression

Once it's green I'll review this. Thank you!

@MaoShizhong
Copy link
Contributor Author

No problem, I'll get that done tomorrow 👍

Copy link
Contributor

github-actions bot commented Oct 31, 2024

Preview URLs

(comment last updated: 2024-11-01 16:13:06)

@wbamberg
Copy link
Collaborator

wbamberg commented Oct 31, 2024

Thanks @MaoShizhong ! This all works and is aligned with the version in the learning-area repo.

The only issue I can see - and sorry I didn't pick up on this before - is that when the source gets Prettier-formatted, the line number and character position change to 87:16. So if someone (like me) has Prettier installed to reformat on save, they will get different values.

I don't know how common this setup is especially for new starters, but it's probably better to use Prettier-formatting, and that is our general standard for code samples on MDN, although learning-area hasn't caught up fully yet.

I appreciate this would mean you doing all this again! If you don't want to just let me know and I'll merge it as is, because it does fix the code injection issue and is an improvement IMO.

@Josh-Cena Josh-Cena requested review from wbamberg and removed request for Josh-Cena October 31, 2024 18:04
@MaoShizhong
Copy link
Contributor Author

@wbamberg Not a problem! I wasn't 100% sure there was a style standard for the learning area (given the original code) so I didn't want to introduce anything more than just using consistent indentation (on top of the tag-moving).

If Prettier is the standard used across MDN then I'm all for using that and redoing the lines here as necessary. Not a problem at all.

@MaoShizhong
Copy link
Contributor Author

MaoShizhong commented Oct 31, 2024

Line number refs and screenshots amended and PR ready for review again (EDIT: I lied - now it is...)

wbamberg pushed a commit to mdn/learning-area that referenced this pull request Nov 1, 2024
## Because

As per
(mdn/content#36541 (comment)), the
MDN code standard uses Prettier. This repo hasn't fully caught up but
this exercise can be formatted as the changes in that PR would reference
this code's line numbers. Using Prettier here would also mean fewer
discrepancies in case any learners happen to already have Prettier
configured (e.g. formatting on autosave) - not too unreasonable of a
situation.

## This PR

- Formats the number game html file with Prettier's default rules.
Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

👍 thank you @MaoShizhong , this looks great!

@wbamberg wbamberg merged commit d51f1d6 into mdn:main Nov 1, 2024
8 checks passed
@MaoShizhong MaoShizhong deleted the what-went-wrong-line-numbers branch November 1, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:Learn:JavaScript Learning area JavaScript docs size/s [PR only] 6-50 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants