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 #8960

Merged
merged 1 commit into from Aug 14, 2017

Conversation

Projects
None yet
3 participants
@GeoffreyBooth
Contributor

GeoffreyBooth commented Jul 28, 2017

This PR splits the coffeescript package into two, mimicking the babel-compiler/ecmascript split:

  • coffeescript-compiler, like babel-compiler, a support package that isolates the logic for compiling CoffeeScript files, including passing through Babel and generating source maps;
  • coffeescript, the build plugin that registers itself as the compiler for CoffeeScript files, and extends CachingCompiler.

This was suggested in a comment, in a discussion about whether or not to move coffeescript into packages/non-core. The result of that discussion was to move into non-core, but unfortunately for this refactor I needed to move these two packages up into packages. When both coffeescript-compiler and coffeescript were in packages/non-core, they couldn’t see each other (#8962). Also, the Meteor test suite ignores packages in packages/non-core, which isn’t good (#8961). These are separate issues related to packages/non-core, not really related to these packages, so I don’t think they need to be resolved in order to accept this PR. Once non-core works the way we want it to, we should move the coffeescript-compiler package back in there, as it’s the package pinned to the version of the NPM coffeescript module. The regular coffeescript package is now more closely tied to caching-compiler, so it might make more sense to keep it as a core package.

I did this so that I could refactor the vue-coffee package, which does its own compilation of CoffeeScript code, to use the code contained in this new coffeescript-compiler package rather than a fork.

@benjamn

This comment has been minimized.

Member

benjamn commented Aug 2, 2017

I've assigned #8961 and #8962 to myself, as I think I have an easy fix.

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Aug 2, 2017

I like easy fixes 😄

Speaking of easy fixes (or maybe not), the CI build fails because Blaze fails to build because spacebars-compiler has a version restriction of coffeescript@1.2.4, and in this PR I bumped that version to 10.0.1:

   While selecting package versions:
   error: Conflict: Constraint coffeescript@1.2.4 is not satisfied by
   coffeescript 10.0.1.
   Constraints on package “coffeescript”:
   * coffeescript@=10.0.1 <- top level
   * coffeescript@1.2.4 <- local-test:spacebars-compiler 1.1.2

Which begs the question, how did this not fail for coffeescript@1.12.6_1 or any of the other 1.x releases? Is semver or the constraint solver not working as I’d expect?

Locally I fixed this by just removing the version constraint in that file, but as it’s in a separate repo that would require a separate PR; and I’m not sure that’s how you’d want to solve it anyway.

benjamn added a commit to meteor/blaze that referenced this pull request Aug 2, 2017

Remove version constraints from spacebars-compiler test dependencies.
These version constraints are unnecessary, since `./meteor test-packages`
runs from a checkout, and all of the depended-on packages can be found in
packages, packages/non-core, or packages/non-core/blaze/packages.

This should fix the problem noted by @GeoffreyBooth in this comment:
meteor/meteor#8960 (comment)
@MaxPleaner

This comment has been minimized.

MaxPleaner commented Aug 5, 2017

Wondering if this will make headway into adding Coffeescript 2 compatibility to meteor or if there is a known workaround?

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Aug 5, 2017

Wondering if this will make headway into adding Coffeescript 2 compatibility to meteor or if there is a known workaround?

That’s where I’m going with this 😄 I think the coffeescript package (which per this PR, becomes just a thin wrapper over CachingCompiler) would be a core package, as CachingCompiler is part of core; and then coffeescript-compiler would be fixed to and updated with each CoffeeScript version. So when 2.0.0 is out, hopefully in a few weeks, coffeescript-compiler gets bumped to 2.0.0_1 etc.

I don’t know how or if a core package can depend on a non-core package and have the tests all work, but hopefully @benjamn’s PRs will make that happen? 😄

@benjamn

This comment has been minimized.

Member

benjamn commented Aug 7, 2017

@GeoffreyBooth Could you try rebasing against devel?

@GeoffreyBooth GeoffreyBooth force-pushed the GeoffreyBooth:coffeescript-compiler-split branch from 487ec7f to 4114675 Aug 7, 2017

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Aug 7, 2017

@benjamn Done. Let me know when/if I should move the coffeescript-compiler package back into non-core. (As in, when it would be possible for a core coffeescript package to find it there.)

@benjamn

This comment has been minimized.

Member

benjamn commented Aug 7, 2017

@GeoffreyBooth I think that's possible now! Still waiting on a fix for the spacebars-compiler version constraint issue, though.

benjamn added a commit to meteor/blaze that referenced this pull request Aug 14, 2017

Remove version constraints from spacebars-compiler test dependencies.
These version constraints are unnecessary, since `./meteor test-packages`
runs from a checkout, and all of the depended-on packages can be found in
packages, packages/non-core, or packages/non-core/blaze/packages.

This should fix the problem noted by @GeoffreyBooth in this comment:
meteor/meteor#8960 (comment)

benjamn added a commit to meteor/blaze that referenced this pull request Aug 14, 2017

Remove version constraints from spacebars-compiler test dependencies. (
…#267)

These version constraints are unnecessary, since `./meteor test-packages`
runs from a checkout, and all of the depended-on packages can be found in
packages, packages/non-core, or packages/non-core/blaze/packages.

This should fix the problem noted by @GeoffreyBooth in this comment:
meteor/meteor#8960 (comment)

@GeoffreyBooth GeoffreyBooth force-pushed the GeoffreyBooth:coffeescript-compiler-split branch 2 times, most recently from ea7f0ec to b1c3ec4 Aug 14, 2017

@benjamn

This comment has been minimized.

Member

benjamn commented Aug 14, 2017

Hey @GeoffreyBooth, thanks for jumping back into this! I rebased these changes just now, and pushed everything except the last commit to release-1.5.2. Would you mind rebasing against release-1.5.2, and changing the base of this pull request to release-1.5.2?

@benjamn

This comment has been minimized.

Member

benjamn commented Aug 14, 2017

I'm going to publish another 1.5.2 beta with the coffeescript@10.0.1 version (with a beta suffix), so we can find out if the constraint solver is happy now. If that doesn't work, we can push the 1.999.1 version change to release-1.5.2 (but I'd like to avoid that unless we have to).

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Aug 14, 2017

@benjamn I discovered that other packages (like Mocha I think it was) also depend on coffeescript@1.x, so 1.999.1 is probably a safer version to use more generally.

@benjamn

This comment has been minimized.

Member

benjamn commented Aug 14, 2017

What about 1.12.x_y, as you had it in 372603f? Are you just worried that this PR is a big change that deserves a major version bump?

In my view, it's acceptable for the Meteor package version to diverge slightly from the npm package version, so 1.13.0 might be another good choice here (no different from 1.999.x, really).

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Aug 14, 2017

No, it’s that the coffeescript-compiler package is the one that imports the NPM coffeescript module, and therefore coffeescript-compiler should be the one that tracks the NPM module’s version (a.k.a. 1.12.7_1 etc.).

The coffeescript package, per this PR, is just a thin wrapper over CachingCompiler, and therefore shouldn’t track the NPM module version. Hence setting coffeescript to 1.999.1 and coffeescript-compiler to 1.12.7_1. Really coffeescript can be anything above 1.12.7 (so that this appears to be an upgrade, for projects already using the current coffeescript package) but below 2.0.0 (or 10.0.0?), since apparently the constraint resolver struggles with the 10.x version.

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Aug 14, 2017

The idea is that coffeescript will rarely need updating, really only when there are breaking changes to CachingCompiler (i.e. so whenever you update ecmascript, you should also update coffeescript). And that can be a core package.

coffeescript-compiler, on the other hand, will update more frequently, with each new release of the NPM coffeescript module. So when CoffeeScript 2.0.0 comes out in a few weeks hopefully, there can be a coffeescript-compiler@2.0.0_1 but coffeescript won’t need to change.

Move coffeescript and coffeescript-test-helper packages back into cor…
…e, as they depend on core modules like CachingCompiler; but leave coffeescript-compiler in non-core, so it can update more frequently as NPM coffeescript gets updated

@GeoffreyBooth GeoffreyBooth force-pushed the GeoffreyBooth:coffeescript-compiler-split branch from b1c3ec4 to 1ba389b Aug 14, 2017

@GeoffreyBooth GeoffreyBooth changed the base branch from devel to release-1.5.2 Aug 14, 2017

@benjamn

This comment has been minimized.

Member

benjamn commented Aug 14, 2017

Thanks for rebasing!

I would have thought that coffeescript and coffeescript-test-helper could depend on caching-compiler even if they weren't in the core packages directory. As I understand it, core packages can't depend on non-core packages, but non-core packages can depend on anything.

If that sounds right to you, I'd like to keep everything in packages/non-core, to give you the freedom to change these packages however you like, without worrying about the Meteor version. But I'm open to whatever you want to do here, and I agree that having the freedom to update coffeescript-compiler is most important.

@benjamn benjamn merged commit eb3c7dd into meteor:release-1.5.2 Aug 14, 2017

9 of 11 checks passed

ci/circleci: Group 6 Your tests failed on CircleCI
Details
ci/circleci: Group 7 Your tests failed on CircleCI
Details
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
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Aug 14, 2017

Oh hey, I’m not sure this was ready to be merged just yet. The code that was merged still had version 10, and coffeescript/coffeescript-test-helpers were in core, which I didn’t think we’d finished discussing. (coffeescript-compiler is non-core, where it should be.)

So about that: currently coffeescript depends on caching-compiler, unversioned (as well as any version of ecmascript and coffeescript-compiler). If coffeescript is a core package, I guess this lack of version specificity is fine, maybe even desired, because it will keep in sync with the version of caching-compiler in core.

If coffeescript becomes a non-core package, we would probably want to fix it to specific versions of caching-compiler and ecmascript—is it even possible to have a build-plugin package using a version of caching-compiler that isn’t the latest version of a particular Meteor version? If it is possible, I guess then that would be desired, as we wouldn’t have to worry about updating coffeescript if the APIs of caching-compiler or ecmascript ever changed. Though on the flip side, I’d kind of prefer MDG to keep responsibility for updating coffeescript whenever caching-compiler or ecmascript got updated, so that coffeescript reaps the benefits of whatever build performance improvements happen in core. This could also become the model to follow for a core TypeScript package; though I don’t know what your plans are for the future regarding core packages and/or NPM.

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Aug 14, 2017

I used this branch to try to run tests in another project, and got this error:

../../meteor/meteor test –driver-package=dispatch:mocha
=> Running Meteor from a checkout – overrides project version (Meteor 1.5.1)
[[[[[ Tests ]]]]]

=> Started proxy.
=> Errors prevented startup:

   While selecting package versions:
   error: Conflict: Constraint coffeescript@1.0.3 is not satisfied by coffeescript 10.0.1.
   Constraints on package “coffeescript”:
   * coffeescript@=10.0.1 <- top level
   * coffeescript@1.0.3 <- practicalmeteor:chai 2.1.0_1
   * coffeescript@1.0.3 <- practicalmeteor:sinon 1.14.1_2

=> Your application has errors. Waiting for file change.

So I guess this is a bug in the constraint resolver? This error went away with the version set to 1.999.1.

@benjamn

This comment has been minimized.

Member

benjamn commented Aug 15, 2017

Sorry about the premature merge!

I do think that coffeescript should impose some constraints on caching-compiler and ecmascript, whether that's implicit because they're all core packages, or explicit in packages/non-core/coffeescript/package.js.

Assuming coffeescript constrains the versions of caching-compiler and ecmascript, the key remaining question is whether the Meteor release should force a particular minimum version of the coffeescript package. My experience has been that new Meteor versions don't tend to rely on specific coffeescript versions, so I don't think Meteor needs to force the coffeescript package to upgrade. If you agree, then I don't see much reason to include coffeescript in the Meteor release, compared to moving it to packages/non-core and letting it constrain the packages it depends on (caching-compiler and ecmascript).

If we update caching-compiler and/or ecmascript in a backwards-incompatible way, we will bump their major versions, which should trigger an update to coffeescript, as long as coffeescript constrains the version of caching-compiler and ecmascript. Likewise, coffeescript can always specify a minimum version of those packages if it wants to take advantage of new functionality (even if it's not a breaking/major change). For example, the processOneFileForTarget functionality in babel-compiler that coffeescript now uses is probably something the coffeescript package should require using an explicit version constraint, regardless of any constraints the Meteor release imposes on the version of babel-compiler.

Note: a version constraint like babel-compiler@x.y.z forces the major version x, and also fixes x.y.z as a minimum version. Contrary to my own assumptions, the minor version y is not fixed, as long as it's >= the minor part of the constraint version. Meteor 1.5.2 introduces a new constraint syntax to ensure major and minor versions match: babel-compiler@~x.y.z (#8991), though you may want to wait until Meteor 1.5.2 is out before using that, so that you can use api.versionsFrom("1.5.2").

It's too bad that pull requests can't be reopened after they've been merged. Could you perhaps open a new PR that moves coffeescript and coffeescript-test-helper back into packages/non-core, and adds version constraints to the dependencies in packages/non-core/coffeescript/package.js? Sorry for all the back-and-forth.

@benjamn

This comment has been minimized.

Member

benjamn commented Aug 15, 2017

One more thought before I go to bed (and sorry again for making this all more complicated than it needed to be): if all three coffeescript* packages live in packages/non-core, we can publish them totally independently from the Meteor release, so I was wrong to ask you to change the base of this PR to release-1.5.2. Instead, I think we should cherry-pick the changes we want back onto a branch based on devel, like you did originally. Once the changes are merged into devel, we can publish the packages whenever you like, without waiting for Meteor 1.5.2.

Good night!

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Aug 15, 2017

I’m happy to open a new PR once we figure out what we’re doing.

If we put all the CoffeeScript packages in non-core, and I constrain the version of caching-compiler, isn’t that in effect constraining the release of Meteor that the coffeescript package can be used with? Since the user presumably can’t install a version of caching-compiler different than the version bundled with whatever Meteor release they’re using. So I’m presented with two unattractive options: leave caching-compiler unconstrained and hope that it doesn’t break coffeescript in the future, or constrain it and then coffeescript needs to be updated every time caching-compiler is, since users will keep upgrading caching-compiler every time they upgrade to a new release.

It’s unfortunate that the constraint resolver doesn’t behave in a “>=” way by default. Looking at the mocha example above, say I release coffeescript@2.0.0; mocha would need a PR to update its version constraint to allow coffeescript@2.0.0 to be used, just as you needed to update Blaze. This is a strong incentive for me to never upgrade coffeescript beyond 1.x, despite the version of the NPM module.

@benjamn

This comment has been minimized.

Member

benjamn commented Aug 15, 2017

I hear what you're saying, but I'm still in favor of packages/non-core/coffeescript.

Considering the alternative, if we put coffeescript in the core packages/ directory, then the Meteor version will always constrain the coffeescript version, which I think is worse than letting the caching-compiler version have an affect the coffeescript version, since caching-compiler doesn't tend to change its version with every Meteor release.

If coffeescript is a core package, then any time you bump the major or minor versions of coffeescript, the new version won't be usable until the next Meteor release, and any packages (e.g. Mocha) that depend on coffeescript will also have to change their constraints when that Meteor release is published. I think that's worse, because it's a stronger incentive for you to never bump the major (or minor) version of coffeescript.

If we do bump the major or minor version of caching-compiler, and you don't consider it a breaking change for coffeescript, you can always just bump your caching-compiler@x.y.z constraint in packages/non-core/coffeescript/package.js, then release a new patch version of coffeescript. The constraint solver will then choose a version of caching-compiler that's compatible with the current Meteor release, and a version of coffeescript that's compatible with that version of caching-compiler. I think this setup makes your work easier, honestly.

(Less important details below; no need to keep reading if you're already convinced… 😅 )

the user presumably can’t install a version of caching-compiler different than the version bundled with whatever Meteor release they’re using

The rules about this have changed over time, so it's worth clarifying. In short, the application developer does have some wiggle room with the caching-compiler version, though that wasn't always the case.

More specifically, if caching-compiler had version a.b.c when the current Meteor release was published, you can use any version a.b.x of caching-compiler with that Meteor release, as long as x >= c. This is the logic that will take effect in Meteor 1.5.2, because core packages will be constrained with the new package@~a.b.c syntax, thanks to #8991. That's slightly more restrictive than the 1.5.1 logic, so I think the new logic is what we should be planning for.

Here's the history of how core package constraints have worked:

  • Before Meteor 1.4, core package constraints were exact (package@=a.b.c), which was way too restrictive.
  • From Meteor 1.4-1.4.2.7 there were no constraints, which was better than exact, but bad in other ways. For example, Meteor could never force a core package to upgrade with a new Meteor release. We're still seeing problems due to this policy, as we release new versions of core packages and people still using 1.4.2.x accidentally upgrade to the latest version.
  • From Meteor 1.4.3 until 1.5.1 (current), you could use any >= version with the same major version. This is how package@a.b.c version constraints work. To be honest, I was surprised to realize (recently) that the minor versions were not constrained, which is why Meteor 1.5.2 will use the package@~a.b.c syntax (#8991).
  • Starting in Meteor 1.5.2 and 1.6, the rules are just like 1.5.1, except that the minor versions of core packages also have to match. That's what the new package@~a.b.c syntax means.

Although core packages will be constrained more tightly in Meteor 1.5.2, the package@a.b.c syntax still has the same meaning it always had, so if you put api.use("caching-compiler@1.1.9") in packages/non-core/coffeescript/package.js, that means your users can use any version of caching-compiler that is >= 1.1.9 and whose major version is also 1, e.g. 1.3.0 or 1.1.10.

I think that's pretty close to the >= policy you're asking for, though I realize it's not exactly what you want, because the major versions have to match.

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

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

Move coffeescript to non-core so we don't have to publish a beta.
@GeoffreyBooth and I are still deciding exactly what to do with the
coffeescript package in #8960, but in the meantime I need to publish
another 1.6 beta, and I'd like to avoid publishing a beta version of
coffeescript along with it.
@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Aug 15, 2017

Okay, how about this:

  • We put all three packages in non-core.
  • Both coffeescript and coffeescript-compiler have the same version. So 1.12.7_1 for now.
  • Whenever coffeescript-compiler gets bumped, coffeescript also gets bumped up, even if nothing changes in coffeescript; and vice versa.
  • coffeescript depends on caching-compiler@1.1.9 and ecmascript@0.8.2 (and no NPM modules).
  • coffeescript-compiler depends on ecmascript@0.8.2, babel-compiler@6.19.4 and the NPM modules coffeescript and source-map.

So this way average users don’t need to know about coffeescript-compiler, just as currently they don’t need to know about babel-compiler: they just install coffeescript, like now, and they’re done. (Just like they install ecmascript now and are done.) By keeping the versions synced, I can tip people off that coffeescript-compiler has updated, even if they have only coffeescript added to their project.

GeoffreyBooth added a commit to GeoffreyBooth/meteor that referenced this pull request Aug 15, 2017

Move coffeescript and coffeescript-test-helper packages back into cor…
…e. (meteor#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.
@benjamn

This comment has been minimized.

Member

benjamn commented Aug 15, 2017

That sounds like it will work! Should coffeescript use an exact version constraint for coffeescript-compiler, so that their versions always match?

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Aug 15, 2017

Yes, sorry I forgot to mention that. See #9018.

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Aug 15, 2017

Originally when I was looking into implementing this, I was trying to use npm-check-versions to just use whatever version of the NPM coffeescript module the project had installed; but I guess that package doesn’t work for build plugins. It would be nice if there was some way to enable that, so that I we don’t need to bump new versions of these packages with every NPM coffeescript release.

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

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.

@GeoffreyBooth GeoffreyBooth referenced this pull request Aug 20, 2017

Merged

Refactor vue-coffee #243

@GeoffreyBooth GeoffreyBooth referenced this pull request Oct 16, 2017

Merged

CoffeeScript 2.0.2 #9107

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