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

Make errors for using advanced JS features friendlier #493

Merged
merged 1 commit into from
Nov 25, 2015

Conversation

MrMagma
Copy link
Contributor

@MrMagma MrMagma commented Nov 14, 2015

This pull request fixes #492 by editing the jshint messages for using ES7 and ES6 features to be worded in a more friendly and helpful way.

Minor Changes

external/jshint/jshint.js

  • Switch W104 from "'some feature' is only available in JavaScript 1.7." to "It looks like you're trying to use 'some feature'. 'some feature' is a JavaScript 1.7 feature and is not currently supported in the live editor."
  • Switch W109 from "'some feature' is only available in ES6 (use esnext option)." to "'some feature' is only available in ES6 which is not supported by the live editor at this point."

If any editing is required please let me know.

Thank you for your time.

@@ -58488,7 +58488,9 @@ var warnings = {
W101: i18n._("Line is too long."),
W102: i18n._("Trailing whitespace."),
W103: i18n._("The '{a}' property is deprecated."),
W104: i18n._("'{a}' is only available in JavaScript 1.7."),
W104: i18n._("It looks like you're trying to use '{a}'. '{a}' is a " +
"JavaScript 1.7 feature and is not currently supported in the live " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you change "live editor" to "this editor" or "this environment"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

@MrMagma
Copy link
Contributor Author

MrMagma commented Nov 16, 2015

@pamelafox I've made the edit you suggested. Let me know if there's anything else I should do.

@@ -58503,7 +58505,8 @@ var warnings = {
W117: i18n._("\"{a}\" is not defined. Make sure you're spelling it correctly " +
"and that you declared it."),
W118: i18n._("'{a}' is only available in Mozilla JavaScript extensions (use moz option)."),
W119: i18n._("'{a}' is only available in ES6 (use esnext option)."),
W119: i18n._("'{a}' is only available in ES6 which is not supported by " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change to environment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Sorry bout that. I do not have access to my desktop currently but once I do I'll fix this and let you know. Sorry for the goof-up.

@GoToLoop
Copy link

I did some looking around and let, const, and destructuring assignments are all 1.7 features, ...

IMO, Khan shouldn't bar ES6 features at all. Specially something as old as const!

@kevinbarabash
Copy link
Contributor

IMO, Khan shouldn't bar ES6 features at all. Specially something as old as const!

It's not a question of barring ES6 features, it's a question of supporting them. We want all CS programs to run cross browser which means that we'd have to babel to compile them. We'd also have to update the linter (or switch to eslint if we want to support all ES6).

@GoToLoop
Copy link

We want all CS programs to run cross browser which means that we'd have to babel to compile them.

  • That all depends on establishing the minimum browser version where full support is guaranteed.
  • Keyword const for example is recognized on very old browsers like Opera 12 and no need for Babel.
  • However, while IE11 works, IE10 doesn't recognize const.
  • But IE browser is gonna be defunct soon and be replaced by Edge for those who prefer MS.
  • It's a pity to bar folks to enjoy some ES6 features when it's already available in modern browsers.
  • And btW, I bet classes gonna bring lotsa Java, C#, Python, etc. programmers to JS camp! :D
    https://developer.Mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes

@MrMagma
Copy link
Contributor Author

MrMagma commented Nov 16, 2015

@pamelafox Sorry for that goof-up I've fixed it now and everything should be ready. Very sorry for that.

@kevinbarabash
Copy link
Contributor

However, while IE11 works, IE10 doesn't recognize const.

While we're dropping IE9 support as of Jan 2016, we'll still be support IE10 for a while.

I bet classes gonna bring lotsa Java, C#, Python, etc. programmers to JS camp! :D

They also make using classes less intimidating for beginner programmers.

@kevinbarabash
Copy link
Contributor

@JavaScriptFTW instead of merging master can you rebase your changes on to master?

@MrMagma
Copy link
Contributor Author

MrMagma commented Nov 24, 2015

@kevinbarabash It's rebased now. It should be ready for merging once the tests are complete.

@MrMagma
Copy link
Contributor Author

MrMagma commented Nov 25, 2015

@kevinbarabash I believe that I've changed the message according to your suggestion.

@kevinbarabash
Copy link
Contributor

Excellent. LGTM.

kevinbarabash added a commit that referenced this pull request Nov 25, 2015
Make errors for using advanced JS features friendlier
@kevinbarabash kevinbarabash merged commit 2e8f662 into Khan:master Nov 25, 2015
@kevinbarabash
Copy link
Contributor

@JavaScriptFTW thanks for the pull request.

@GoToLoop
Copy link

While we're dropping IE9 support as of Jan 2016, we'll still be support IE10 for a while.

Seems like not even MS wants to support those very old browsers anymore after January 12, 2016: :-D
https://www.Microsoft.com/en-us/WindowsForBusiness/End-of-IE-support

For now IE11 survives. But I bet it's not gonna take too long for MS to support Edge only... @_@

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unhelpful Error Message When Using ES6/7 Features
4 participants