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

CoffeeScript 2 #30

Merged
merged 4 commits into from
Mar 28, 2017
Merged

CoffeeScript 2 #30

merged 4 commits into from
Mar 28, 2017

Conversation

jalavosus
Copy link
Contributor

Basic changes to (hopefully) support coffeescript 2.

Essentially hoping that it doesn't require much besides requiring "coffeescript" instead of "coffee-script" and package.json requiring "coffeescript" at any version.

@jsf-clabot
Copy link

jsf-clabot commented Mar 2, 2017

CLA assistant check
All committers have signed the CLA.

@michael-ciniawsky michael-ciniawsky self-assigned this Mar 9, 2017
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@jalavosus That one is actually a bit tricky, because the loader should support coffeescript v1x && v2x and folks are normally able to via peerDependencies, but since the package name changed it won't work

@@ -2,7 +2,7 @@
MIT License http://www.opensource.org/licenses/mit-license.php
Author Tobias Koppers @sokra
*/
var coffee = require("coffee-script");
var coffee = require("coffeescript");
Copy link
Member

@michael-ciniawsky michael-ciniawsky Mar 9, 2017

Choose a reason for hiding this comment

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

Maybe wrap in a try/catch ? 😛 I'm not sure...

Copy link

Choose a reason for hiding this comment

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

No need. Peer dep should force it. And it's better to fail hard in this case most likely.

@joshwiens
Copy link
Member

@jalavosus - This is going to take some internal discussion on how we are going to handle this. Give us a day or two and we will get you sorted out one way or another.

@jalavosus
Copy link
Contributor Author

I'll take a look into the try-catch option when I get back to my computer.

Can the package.json require both the old and new versions to be installed separately?

@michael-ciniawsky
Copy link
Member

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Mar 13, 2017

@jalavosus It's safe to just go with coffeescript for v1. >= v1.8.x will be included in the new package and v2 is at coffeescript@beta atm. So please rebase and update to coffeescript 😛

@jalavosus
Copy link
Contributor Author

jalavosus commented Mar 13, 2017 via email

@michael-ciniawsky
Copy link
Member

@jalavosus I'm waiting for final confirmation in jashkenas/coffeescript#4462, but yes setting coffeescript as peerDependency and require('coffeescript') should be fine, no rush you can also wait for confirmation, if you don't want to spend effort on it without 😛

package.json Outdated
@@ -7,7 +7,7 @@
"loader-utils": "^1.0.2"
},
"peerDependencies": {
"coffee-script": "1.x"
"coffeescript": "*"
Copy link

Choose a reason for hiding this comment

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

The risk with this is that if they break their API in a major, the loader will break. So change to a range if you want to be careful.

Copy link
Member

@michael-ciniawsky michael-ciniawsky Mar 13, 2017

Choose a reason for hiding this comment

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

Yep, we need to test for breaking Node API changes before release

{
  "peerDependencies": {
     "coffeescript": ">= 1.8.x"
   }
}

@michael-ciniawsky michael-ciniawsky changed the title coffeescript2 changes CoffeeScript 2 Mar 13, 2017
@michael-ciniawsky michael-ciniawsky mentioned this pull request Mar 21, 2017
@michael-ciniawsky
Copy link
Member

Closes #33

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@jalavosus Please fix the peerDependency range and we are good to go

{
  "peerDependencies": {
     "coffeescript": ">= 1.8.x || >= 2.x"
   }
}

@jalavosus
Copy link
Contributor Author

Updated package.json, sorry for the holdup.

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

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