Closed zapu closed 8 years ago
I'm not quite sure if we want a branch of each release. Maybe the easiest thing to do is to make a lib-iced
parallel to lib
and then to reference that in our iced releases, and we don't have conflicts when we merge with mainline. What do you think?
So the additional files in lib-iced
would try to e.g. patch prototypes of classes in nodes.coffee
with additional functions or other changes, so the nodes.coffee
would remain unchanged compared to mainline coffee?
No, I was thinking the output JS would go into lib-iced
. We'd still have a fork of nodes.coffee
in the original src/
location, but I think that's what we want --- we want to have to resolve conflicts on merges. It's the compiled output files that we shouldn't be dealing with merge conflicts in.
That makes sense, right. Sorry, I didn't have the source open and mixed up lib
and src
.
Sounds good! Also, what would git do when lib
was in .gitignore
?
I think the coffee people (and maybe us) want those auto-gen files check-in, so you can run the code without having to compile it, and maybe so you know exactly what was checked in as part of a release. So I think we should continue to do so.
Oh, but I thought about having everything in lib-iced
(for reasons above + easier merge) and removing lib
(for shorter install? maybe)
The advantage of having lib
there is that everything will compile out of the box (albeit with old coffee). And we'll have tons of merge errors as we merge with mainline coffee script if we remove it from the repo.
In other words:
src/
- patched src files
lib/coffee-script
- original Coffee output files from mainline
lib-iced/coffee-script
- IcedCoffeeScript files compiled from src/
Looks good. Should I introduce this change in this pull request?
Maybe a new PR, let's get that one in, and then circle back to this one? Many thanks @zapu, sorry I didn't think of this sooner, but I think it will simplify the workflow and make merges easier (good for everyone involved!!)
BTW, looks like we can leave the lib/
files out of package.json
Ok. After that I'll try to merge in latest coffee, I think it moved forward a little bit and there has never been a merge since the start of iced3 work.
Cool!
Updated this to support last lib-iced
directory changes.
I'm ready to give this a whirl, what do you think?
Sure! Hope it works. Please check if I got the versioning correctly, though.
Definitely ran correctly, but three things I noticed:
./bin/cake test
errors on 2 test cases (I assume this is known)// Generated by IcedCoffeeScript 1.10.0
)-v
is wrong; it shows up as: CoffeeScript version 1.10.0
thanks for all the great work btw!
Oh right, hm... Thanks, I completely missed these.
What test failures did you get? empty command evaluates to undefined
and SourceMap tests
?
yes both of those....
They are both "inherited" from coffeescript :) "empty command evaluates..." is dependent on node version, I reported this but nothing really came out of it. The second one got fixed here: https://github.com/jashkenas/coffeescript/commit/d7e752bc5db31bcba940c46bb77305c3ea6251d8 We could merge this.
Looks like iced2
had the "iced" version in the coffee-script.coffee
file ( https://github.com/maxtaco/coffee-script/blob/iced2/src/coffee-script.coffee#L17 ) while I tried to preserve "coffeescript version" there and have the "iced patch number" separately. What would be a good approach there?
The least hacky approach and the approach most amenable to changes from master is:
exports.VERSION
we get from master in coffee-script.coffee
iced.coffee
source file, and add a exports.PATCH number in iced.coffee
exports.icedCoffeeScriptVersion()
function in iced.coffee
that computes the version as function of 2 and 3.exports.icedCoffeeScriptVersion()
?Great, that's something that I would do as well. Thanks
👍
With additional file there is the issue of cyclic dependency, because coffee-script.coffee
needs iced.coffee
to make the "Generated by ..." string, but iced.coffee
needs coffee-script.coffee
to check VERSION and generate version string of its own. This can be easily solved by late-require, but still.
Also, the "header (#1778)"
test in compilation.coffee
will fail during browser tests, because it would need to require iced.coffee
to get the version and compare to the one in generated header, and there is no "require" during test:browser.
Ah, makes sense. Ok, would it help to just move everything into coffee-script.coffee
? That shouldn't be too much a problem on a merge... Thanks!
A release tool as discussed in #180
Here is what I have so far. It does not switch branches though, but commits and tags in current branch. It also does not do any pulls or merges. Do we really want a branch for each release? Maybe I'm not understanding the whole workflow here.