/meteor

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

Split coffeescript package, part 2 #9018

Merged
merged 10 commits into from Aug 17, 2017

Conversation

Reviewers

@benjamn benjamn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
  Release 1.5.2
2 participants
@GeoffreyBooth @benjamn
@GeoffreyBooth
Contributor

GeoffreyBooth commented Aug 15, 2017

See #8960.

Geoffrey Booth and others added some commits Jul 26, 2017

Geoffrey Booth @GeoffreyBooth
Instructions for how to test the coffeescript package
0d65440
Geoffrey Booth @GeoffreyBooth
Bump CoffeeScript version
7b8a5f3
Geoffrey Booth @GeoffreyBooth
Split coffeescript package into coffeescript / coffeescript-compiler.
e4c0240
@GeoffreyBooth
Move coffeescript and coffeescript-test-helper packages back into cor… 
…e. (#8960)

They depend on core packages like caching-compiler, but coffeescript-compiler
can remain in non-core, so it can update more frequently as npm coffeescript
gets updated.
13fb390
@benjamn @GeoffreyBooth
Bump coffeescript package version to 1.13.0. 
This Meteor package version does not need to track the npm version of the
coffeescript package, and probably should not change as often as the
version of the packages/non-core/coffeescript-compiler package.
3677e17
@GeoffreyBooth
Move all coffeescript* packages back into non-core
dd6a0f7
@GeoffreyBooth
Fix `coffeescript` package version to same as `coffeescript-compiler`… 
…; update documentation
89af934
@GeoffreyBooth
Fix coffeescript / coffeescript-compiler dependency versions
Loading status checks…
27c6a52

@GeoffreyBooth GeoffreyBooth referenced this pull request Aug 15, 2017

Merged

Split coffeescript package #8960

@benjamn

benjamn reviewed Aug 15, 2017
View changes

packages/non-core/coffeescript-compiler/package.js Outdated
api.use('babel-compiler@6.19.4');
api.use('ecmascript@0.8.2');

api.addFiles(['coffeescript-compiler.js'], 'server');

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Aug 15, 2017

Member

Since you're using ecmascript, I would make this

api.mainModule("coffeescript-compiler.js", "server");
packages/non-core/coffeescript-compiler/coffeescript-compiler.js Outdated


// The CompileResult for this CachingCompiler is a {source, sourceMap} object.
CoffeeScriptCompiler = class CoffeeScriptCompiler {

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Aug 15, 2017

Member

If you use api.mainModule in package.js, then this line can just be

export class CoffeeScriptCompiler {

and api.export("CoffeeScriptCompiler", "server") will use the exported CoffeeScriptCompiler.
packages/non-core/coffeescript/package.js
@@ -1,29 +1,29 @@
Package.describe({
summary: "Javascript dialect with fewer braces and semicolons",
version: "1.12.6_1"
name: 'coffeescript',

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Aug 15, 2017

Member

Glad to have a name here now!
packages/non-core/coffeescript/package.js Outdated
"source-map": "0.5.6"
}
name: 'compile-coffeescript',
use: ['caching-compiler@1.1.9', 'ecmascript@0.8.2', 'coffeescript-compiler@1.12.7_1'],

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Aug 15, 2017 •
edited

Member

For the record, this means an application developer who's using coffeescript can use any version of caching-compiler that's >= 1.1.9 and has the same major version (1.x.y). Likewise, any version of ecmascript is fair game if it starts with a 0. and is more recent than 0.8.2.

Should we make the coffeescript-compiler version constraint exact (@=1.12.7_1)? Do you think users might ever want to use different versions of coffeescript and coffeescript-compiler?

@benjamn benjamn added this to the Release 1.5.2 milestone Aug 15, 2017

@benjamn

This comment has been minimized.

Sign in to view
Member

benjamn commented Aug 15, 2017

Looks like Circle CI is having problems right now, by the way: https://status.circleci.com/
@GeoffreyBooth

This comment has been minimized.

Sign in to view
Contributor

GeoffreyBooth commented Aug 16, 2017

@benjamn I’ve revised per your notes.
@benjamn

benjamn requested changes Aug 16, 2017
View changes

packages/non-core/coffeescript-compiler/package.js Outdated
api.use('babel-compiler@6.19.4');
api.use('ecmascript@0.8.2');

api.mainModule(['coffeescript-compiler.js'], 'server');

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Aug 16, 2017

Member

This line is causing some legit test failures. It should just be a string, not an array (as there can be only one main module):

api.mainModule('coffeescript-compiler.js', 'server');

I tried pushing a commit to this branch, but I think only you can do that, @GeoffreyBooth.

This comment has been minimized.

Sign in to view
Copy link
@GeoffreyBooth

GeoffreyBooth Aug 16, 2017

Contributor

Updated. I have “allow edits from maintainers” checked, so I’d think you should be able to push commits . . .
Geoffrey Booth
Fix syntax of api.mainModule
Loading status checks…
4bd17e3
@GeoffreyBooth

This comment has been minimized.

Sign in to view
Contributor

GeoffreyBooth commented Aug 17, 2017

Looks like the build passed!
@benjamn

benjamn approved these changes Aug 17, 2017
View changes

@benjamn benjamn merged commit 568e695 into meteor:devel Aug 17, 2017
11 checks passed

11 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Group 7 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benjamn

This comment has been minimized.

Sign in to view
Member

benjamn commented Aug 17, 2017

Are you able to republish the packages, or should I?
@GeoffreyBooth

This comment has been minimized.

Sign in to view
Contributor

GeoffreyBooth commented Aug 17, 2017

I don't think I am . . . ?
@benjamn

This comment has been minimized.

Sign in to view
Member

benjamn commented Aug 17, 2017

Are you fine with me publishing them now, then?
@GeoffreyBooth

This comment has been minimized.

Sign in to view
Contributor

GeoffreyBooth commented Aug 17, 2017

Of course.

benjamn added a commit that referenced this pull request Aug 17, 2017

@benjamn
Specify a few more version constraints in coffeescript packages. 
Small follow-up to #9018.

Note (especially @GeoffreyBooth): these version constraints do two things:
(1) specify a minimum version, and (2) fix the major version. In other
words, the coffeescript package should not need to be republished when we
publish new minor versions of these core packages in Meteor 1.5.2 or 1.6.

When/if we publish a new major version of these packages, the coffeescript
package can simply bump its version constraints, but that probably won't
happen any time soon. I think that's reasonable because a major version
bump suggests there are some significant changes that need to be
acknowledged by dependent packages.
Verified
This commit was signed with a verified signature.
@benjamn benjamn Ben Newman
GPG key ID: 15D2E57778C1A4BF Learn about signing commits
Loading status checks…
65c8b48
@benjamn

This comment has been minimized.

Sign in to view
Member

benjamn commented Aug 17, 2017

Okay! All three packages have been published, and I can confirm that I get the new versions when I run meteor add coffeescript in a new 1.5.1 application:

1:~/dev/coffee-split-test% ~/.meteor/meteor --version
Meteor 1.5.1
1:~/dev/coffee-split-test% ~/.meteor/meteor add coffeescript

Changes to your project's package version selections:

coffeescript           added, version 1.12.7_1
coffeescript-compiler  added, version 1.12.7_1

coffeescript: Javascript dialect with fewer braces and semicolons

benjamn added a commit that referenced this pull request Aug 17, 2017

@benjamn
Undo coffeescript-related changes made on release-1.5.2. 
This will make it easier to merge devel into release-1.5.2, since devel
now contains the final verison of these changes, as implemented by
@GeoffreyBooth in #9018.

Revert "Bump coffeescript package version to 1.13.0."
This reverts commit d727ad0.

Revert "Move coffeescript and coffeescript-test-helper packages back into core. (#8960)"
This reverts commit eb3c7dd.

Revert "Split coffeescript package into coffeescript / coffeescript-compiler."
This reverts commit 8344cbf.

Revert "Instructions for how to test the coffeescript package"
This reverts commit 491cbc3.
Verified
This commit was signed with a verified signature.
@benjamn benjamn Ben Newman
GPG key ID: 15D2E57778C1A4BF Learn about signing commits
70ab06a
@GeoffreyBooth

This comment has been minimized.

Sign in to view
Contributor

GeoffreyBooth commented Aug 17, 2017

Thanks!
@benjamn

This comment has been minimized.

Sign in to view
Member

benjamn commented Aug 17, 2017

Thank you for all your work on this, @GeoffreyBooth!

@GeoffreyBooth GeoffreyBooth referenced this pull request Aug 20, 2017

Merged

Refactor vue-coffee #243

@GeoffreyBooth GeoffreyBooth deleted the GeoffreyBooth:coffeescript-compiler-split-2 branch Aug 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment