fix: error while installing [email protected] 'invalid or unexpect…

[alan-agius4]

…ed token'

Closes: #4805

@vendethiel, @lydell & @GeoffreyBooth.

[Alevale]

This will actually show the message for anybody using coffeescript, no matter if using the "coffeescript" package or "coffee-script" so I would say it's not valid because what you want is to say that you need to change the name to "coffeescript" only if you are using "coffee-script" and not also if you are using it.

[alan-agius4]

@Alevale, the if statement is based on the "name" value in the package.json which in package.json is always coffee-script

https://github.com/jashkenas/coffeescript/blob/1/package.json#L2

Unless I am missing something

in the coffescript package there is no postinstall script
https://github.com/jashkenas/coffeescript/blob/master/package.json

[GeoffreyBooth]

I tested this in Windows by pasting the package.json into a folder and running npm run postinstall and it seems to work. It’s unfortunate that we lose the quotes around "coffee-script", as that was meant to imply that string in package.json, but I can live with that.

@alan-agius4 have you tested this in Windows?

[GeoffreyBooth]

Oh and I’ll just leave out the postinstall entirely in the coffeescript side. The if was meant so that there would be one consistent codebase between the two, but clearly it’s not worth the headache.

[alan-agius4]

Yeah my screenshot i posted is on windows

…

On Thu, 30 Nov 2017 at 17:21, Geoffrey Booth ***@***.***> wrote: I tested this in Windows by pasting the package.json into a folder and running npm run postinstall and it seems to work. It’s unfortunate that we lose the quotes around "coffee-script", as that was meant to imply that string in package.json, but I can live with that. @alan-agius4 <https://github.com/alan-agius4> have you tested this in Windows? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4807 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQv-Wl39HEDAzFHHy2jZAYsPsnb_JmBpks5s7tXfgaJpZM4QwcWY> .

[GeoffreyBooth]

Great, do you mind really and truly testing it via the steps in #4759 (comment)? I just don’t want to push more than one fix to this 😄 And I don’t have a great Windows environment to test with.

(Check out your branch rather than redirect as mentioned in the instructions.)

[alan-agius4]

Unfortunately i am at home nowand I only have mac available.

Basicly my test was the following;
npm pack (this created acts the same as npm publish but having the same pkg on disk rather than npm repo)

And

npm install

Anyone willing to test it out your way?

[GeoffreyBooth]

@alan-agius4 If there’s a way to have this only print on direct installs, we might as well add that ability at the same time as we fix this. I like your code in #4805 (comment), but it needs to be rewritten to run in Node 0.8+ (so no const etc.) and anything risky should go in a try block. We could put it into a warning.js and change postinstall to node ./warning.js. That should also fix the Windows quotes issue.

[alan-agius4]

Sure thing, though i’ll only be available to do it tomorrow. Lets say in 14hrs. Is it okay for you?

…

On Thu, 30 Nov 2017 at 18:53, Geoffrey Booth ***@***.***> wrote: @alan-agius4 <https://github.com/alan-agius4> If there’s a way to have this only print on direct installs, we might as well add that ability at the same time as we fix this. I like your code in #4805 (comment) <#4805 (comment)>, but it needs to be rewritten to run in Node 0.8+ (so no const etc.) and anything risky should go in a try block. We could put it into a warning.js and change postinstall to node ./warning.js. That should also fix the Windows quotes issue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4807 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQv-Wgh8btF0Y3DywAXc9XhxGtMGuR00ks5s7uuvgaJpZM4QwcWY> .

[GeoffreyBooth]

Yes, I don’t want to try again until Friday night Pacific time at the earliest.

[alan-agius4]

@GeoffreyBooth I pushed the implementation that we discussed yesterday, that said I had to do some modifications to the original snippet to get it working.

[GeoffreyBooth]

Can you explain this?

[alan-agius4]

In npm when a package is installed the package.json is amended and some new fields are added one of these fields is _where which indicates from where the package was installed.

In case _where is not defined we cannot determine from where it was installed, thus show the warning.

[GeoffreyBooth]

Also there are some misspellings: hasOldCoffeeScript, pkgData.

[GeoffreyBooth]

Let's pause on this, per #4714 (comment). I think we might just use npm deprecate instead after all.

[GeoffreyBooth]

Closing as @jashkenas will npm deprecate per #4714 (comment).