Skip to content

CoffeeScript 2#30

Merged
michael-ciniawsky merged 4 commits intowebpack:masterfrom
jalavosus:cs2
Mar 28, 2017
Merged

CoffeeScript 2#30
michael-ciniawsky merged 4 commits intowebpack:masterfrom
jalavosus:cs2

Conversation

@jalavosus
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor

@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

Comment thread index.js
Author Tobias Koppers @sokra
*/
var coffee = require("coffee-script");
var coffee = require("coffeescript");
Copy link
Copy Markdown
Contributor

@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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

@michael-ciniawsky
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

jalavosus commented Mar 13, 2017 via email

@michael-ciniawsky
Copy link
Copy Markdown
Contributor

@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 😛

Comment thread package.json Outdated
},
"peerDependencies": {
"coffee-script": "1.x"
"coffeescript": "*"
Copy link
Copy Markdown

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
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

Closes #33

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor Author

Updated package.json, sorry for the holdup.

Copy link
Copy Markdown
Contributor

@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