Skip to content

fix#4805: in single and double quotes #4809

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

Closed
wants to merge 3 commits into from

Conversation

hajjem-ayoub
Copy link

@hajjem-ayoub hajjem-ayoub commented Nov 30, 2017

fix for #4805

@hajjem-ayoub hajjem-ayoub changed the title fix#4807: in single and double quotes fix#4805: in single and double quotes Nov 30, 2017
package.json Outdated
@@ -28,7 +28,7 @@
"repl.js"
],
"scripts": {
"postinstall": "node --eval 'if (require(\"./package.json\").name === \"coffee-script\") { var red, yellow, cyan, reset; red = yellow = cyan = reset = \"\"; if (!process.env.NODE_DISABLE_COLORS) { red = \"\\x1b[31m\"; yellow = \"\\x1b[33m\"; cyan = \"\\x1b[36m\"; reset = \"\\x1b[0m\"; } console.warn(red + \"CoffeeScript has moved!\" + reset + \" Please update references to \" + yellow + \"\\\"coffee-script\\\"\" + reset + \" to use \" + yellow + \"\\\"coffeescript\\\"\" + reset + \" (no hyphen) instead.\"); console.warn(\"Also, a new major version has been released under the \" + yellow + \"coffeescript\" + reset + \" name on NPM. This new release targets modern JavaScript, with minimal breaking changes. Learn more at \" + cyan + \"http://coffeescript.org\" + reset + \".\"); console.warn(\"\"); }'",
"postinstall": "node --eval \"if (require('./package.json').name === 'coffee-script') { var red, yellow, cyan, reset; red = yellow = cyan = reset = ''; if (!process.env.NODE_DISABLE_COLORS) { red = '\\x1b[31m'; yellow = '\\x1b[33m'; cyan = '\\x1b[36m'; reset = '\\x1b[0m'; } console.warn(red + 'CoffeeScript has moved!' + reset + ' Please update references to ' + yellow + '\\\"coffee-script\\\"' + reset + ' to use ' + yellow + '\\\"coffeescript\\\"' + reset + ' (no hyphen) instead.'); console.warn('Also, a new major version has been released under the ' + yellow + 'coffeescript' + reset + ' name on NPM. This new release targets modern JavaScript, with minimal breaking changes. Learn more at ' + cyan + 'http://coffeescript.org' + reset + '.'); console.warn(''); }\"",
Copy link
Collaborator

Choose a reason for hiding this comment

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

have you tried \\\"coffee-script\\\" and confirm it works on both platforms?

Choose a reason for hiding this comment

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

@hajjem-ayoub Does it works on your local with this fix?

Copy link
Author

Choose a reason for hiding this comment

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

yes tried it only with windows VM, i can also check on mac

Copy link

@rossanmol rossanmol Nov 30, 2017

Choose a reason for hiding this comment

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

On mine doesn't work. (Windows 10)

Copy link
Author

Choose a reason for hiding this comment

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

i also tried to npm link it in mac and it seems to work as well, it shows the message correctly at least

Copy link
Author

Choose a reason for hiding this comment

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

@rossanmol just to debug it, can you tell me how did you test it?

Copy link

@rossanmol rossanmol Nov 30, 2017

Choose a reason for hiding this comment

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

run node --eval in cmd, might be that it works only with npm postinstall

@rossanmol
Copy link

rossanmol commented Nov 30, 2017

What about this solution:

node --eval "if (require(`./package.json`).name === `coffee-script`) { var red, yellow, cyan, reset; red = yellow = cyan = reset = ``; if (!process.env.NODE_DISABLE_COLORS) { red = `\\x1b[31m`; yellow = `\\x1b[33m`; cyan = `\\x1b[36m`; reset = `\\x1b[0m`; } console.warn(red + `CoffeeScript has moved!` + reset + ` Please update references to ` + yellow + `\\\"coffee-script\\\"` + reset + ` to use ` + yellow + `\\\"coffeescript\\\"` + reset + ` (no hyphen) instead.`); console.warn(`Also, a new major version has been released under the ` + yellow + `coffeescript` + reset + ` name on NPM. This new release targets modern JavaScript, with minimal breaking changes. Learn more at ` + cyan + `http://coffeescript.org` + reset + `.`); console.warn(``); }"

@vendethiel
Copy link
Collaborator

@rossanmol people using 1.x not all have access to ES6+ features. That's kinda the point of the 1.x branch.

@hajjem-ayoub
Copy link
Author

@rossanmol you cannot use ` you can only use ' or " because es6 not everywhere, but i will try to look for windows 10 to check, but can you tell me how you tested??
did you pull the code and npm linked it??

@rossanmol
Copy link

@hajjem-ayoub run the following in cmd:

node --eval \"if (require('./package.json').name === 'coffee-script') { var red, yellow, cyan, reset; red = yellow = cyan = reset = ''; if (!process.env.NODE_DISABLE_COLORS) { red = '\\x1b[31m'; yellow = '\\x1b[33m'; cyan = '\\x1b[36m'; reset = '\\x1b[0m'; } console.warn(red + 'CoffeeScript has moved!' + reset + ' Please update references to ' + yellow + '\\\"coffee-script\\\"' + reset + ' to use ' + yellow + '\\\"coffeescript\\\"' + reset + ' (no hyphen) instead.'); console.warn('Also, a new major version has been released under the ' + yellow + 'coffeescript' + reset + ' name on NPM. This new release targets modern JavaScript, with minimal breaking changes. Learn more at ' + cyan + 'http://coffeescript.org' + reset + '.'); console.warn(''); }\"

I didn't try linking it.

@hajjem-ayoub
Copy link
Author

@rossanmol yes but than you don't have process.env.NODE_DISABLE_COLORS

@rossanmol
Copy link

@hajjem-ayoub it fails because of the if statement, same as before.

@hajjem-ayoub
Copy link
Author

just pushing one like that now :D

@hajjem-ayoub
Copy link
Author

@rossanmol and for coloring i used chalk

@vendethiel
Copy link
Collaborator

@hajjem-ayoub if anything, you missed a git add.

@hajjem-ayoub
Copy link
Author

hajjem-ayoub commented Nov 30, 2017

@vendethiel sorry for that, i amended my commit and push forced again
let me know if i need to add the disable color

@hajjem-ayoub hajjem-ayoub force-pushed the 1.12.8-branch branch 4 times, most recently from 1f1477b to 2f8bb86 Compare November 30, 2017 15:47
@hajjem-ayoub
Copy link
Author

@vendethiel done with this and it looks like working fine, i just had to add chalk to dependancies if that's ok

@vendethiel
Copy link
Collaborator

I don't think coloring is important enough to warrant adding a dep.

@GeoffreyBooth
Copy link
Collaborator

Yes, we don’t have dependencies. I think #4807 is closer to what we need to do.

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.

5 participants