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

v1.14/v1.15 includes a breaking change in a minor version. #433

Closed
ljharb opened this issue Aug 31, 2018 · 16 comments
Closed

v1.14/v1.15 includes a breaking change in a minor version. #433

ljharb opened this issue Aug 31, 2018 · 16 comments
Labels

Comments

@ljharb
Copy link

ljharb commented Aug 31, 2018

v1.14 works on node < 4; v1.15 does not. This is a major change.

Please revert and publish a v1.15 patch; and if you'd like to drop node < 4, please republish as a v2.

See https://travis-ci.org/es-shims/es5-shim/jobs/423131852 for an example.

EDIT by @brodybits: the breaking change is in 7ad50c8:

--- a/package.json
+++ b/package.json
@@ -25,7 +25,7 @@
   "dependencies": {
     "coffee-script": ">=1.0.1",
     "jasmine-reporters": "~1.0.0",
-    "jasmine-growl-reporter": "~0.0.2",
+    "jasmine-growl-reporter": "~1.0.1",
     "requirejs": ">=0.27.1",
     "walkdir": ">= 0.0.1",
     "underscore": ">= 1.3.1",

See below and es-shims/es5-shim@5a27afb for explanation.

Second possibly breaking change was the following (already fixed now):

   "dependencies": {
-    "coffee-script": ">=1.0.1",
+    "coffeescript": ">=1.0.1",
     "jasmine-reporters": "~1.0.0"
@ljharb
Copy link
Author

ljharb commented Aug 31, 2018

k turns out it’s not in this project, but a dep: https://travis-ci.org/es-shims/es5-shim/builds/423137988

I’ll debug more later today; either the dep needs to get fixed or jasmine-node needs to pin it, whichever it is.

@brodybits brodybits added the Bug label Aug 31, 2018
@brodybits
Copy link
Collaborator

Thanks for the update. From 1.14.x...master the only breaking change I can see is in package.json:

   "dependencies": {
-    "coffee-script": ">=1.0.1",
+    "coffeescript": ">=1.0.1",
     "jasmine-reporters": "~1.0.0"

which has the same effect as using coffeescript@2 (until they publish another major release). Now labeled as a bug, hope to publish an update sometime next week.

As a side point I would rather fix this one thing than revert the whole change in 7aa3b53. Less messy in the history (IMHO). Another side point is that there is already a "v2" branch (https://github.com/mhevery/jasmine-node/tree/Jasmine2.0) that seems to both have new features and be outdated.

brodybits pushed a commit to brodybits/jasmine-node that referenced this issue Aug 31, 2018
to avoid possibly breaking change in minor release

(version 1.15.1)

Ref: mhevery#433
brodybits pushed a commit to brodybits/jasmine-node that referenced this issue Aug 31, 2018
to avoid potentially breaking change in a minor release

(version 1.15.1)

Ref: mhevery#433
brodybits pushed a commit to brodybits/jasmine-node that referenced this issue Aug 31, 2018
to avoid potentially breaking change in a minor release

and mark version 1.15.1

ref: mhevery#433
@brodybits
Copy link
Collaborator

brodybits commented Aug 31, 2018

I just published pushed version 1.5.1 which uses coffeescript@1 in order to remove possibly breaking change from version 1. (My bad: I discovered that I did not update version in package.json, missed the npm publish error message. I just removed the 1.5.1 tag; update is now published as 1.6.0 as described below.)

Some general comments:

I would recommend transitioning from jasmine-node to jasmine whenever possible. I took over maintenance (not ownership) of this project for the sake of projects that are not yet ready to transition for some reason. Motivation for me is cordova-js (we would need to change the Cordova JavaScript, which is not wanted in a minor release). I am guessing es5-shim would have a similar motivation.

FYI both 1.5.0 and 1.5.1 seem to work on Node.js 4. So I suspect there is something really strange going on in es5-shim. Maybe with dependencies, maybe something else. But I am really happy that the reported observations led to the discovery of unwanted coffeescript upgrade in dependencies.

Another side point I discovered is that this project does not have anything like Travis CI enabled. Considering that this project is really in a "maintenance mode" I will probably leave this part at a very low priority.

@brodybits
Copy link
Collaborator

I just published version 1.6.0 which uses coffeescript@1, fixes some other dependencies to avoid major package updates, and adds a note that this package is in maintenance mode. Best of luck to @ljharb in solving the problem with es5-shim for good. Closing now.

@ljharb
Copy link
Author

ljharb commented Aug 31, 2018

Thanks, I appreciate the effort - I'll post here if i think there's anything further for jasmine-node to do.

@ljharb
Copy link
Author

ljharb commented Aug 31, 2018

Found the source: tj/node-growl#79 via jasmine-growl-reporter (AlphaHydrae/jasmine-growl-reporter#3)

@ljharb
Copy link
Author

ljharb commented Aug 31, 2018

One possible change here would be to drop the jasmine-growl-reporter dep from ~1 back to ~0.2 - that seems to have been the trigger :-/

ljharb added a commit to es-shims/es5-shim that referenced this issue Aug 31, 2018
jasmine-node v1.14+ depends on ~1.0.1 of jasmine-growl-reporter: mhevery/jasmine-node#433
jasmine-growl-reporter v1.0.1 depends on growl ^1.10.2: AlphaHydrae/jasmine-growl-reporter#3
growl v1.10.1+ has a breaking change: tj/node-growl#79
@brodybits brodybits changed the title v1.15 includes a breaking change in a minor version. v1.14/v1.15 includes a breaking change in a minor version. Sep 4, 2018
@brodybits
Copy link
Collaborator

@ljharb I would definitely see the following change in 7ad50c8 as a breaking change:

--- a/package.json
+++ b/package.json
@@ -25,7 +25,7 @@
   "dependencies": {
     "coffee-script": ">=1.0.1",
     "jasmine-reporters": "~1.0.0",
-    "jasmine-growl-reporter": "~0.0.2",
+    "jasmine-growl-reporter": "~1.0.1",
     "requirejs": ">=0.27.1",
     "walkdir": ">= 0.0.1",
     "underscore": ">= 1.3.1",

I wish you would have reported this observation in a new, separate issue. Considering that you have already referenced this issue from some other places, I am am reopening it with updated title description to reflect the new information. I will try to resolve this one along with #435 (use tilde instead of carrot) in a patch today.

And yes, you can definitely blame me for the breaking changes here😒

@ljharb
Copy link
Author

ljharb commented Sep 4, 2018

Thanks, since it was the same root problem I figured I'd post it here; i'm happy to file a new one if you like.

@brodybits
Copy link
Collaborator

Let's keep it here. You were right: evidence of the root problem was already given in the description.

I think part of the confusion was that the breaking change in 7ad50c8 was in 1.14.6, which was not marked so clearly, and caused the trouble in es-shims/es5-shim@37e7f3e.

@brodybits
Copy link
Collaborator

I would also like to see a quick, easy-to-read description of npm semver etiquette that we can point people to, to help avoid this kind of issue in the future. I just raised the request in semver/semver#461.

@brodybits
Copy link
Collaborator

brodybits commented Sep 4, 2018

node lib/jasmine-node/cli.js --growl spec mostly passes on Node.js 4.0, 0.12, and 0.10 if I checkout version 1.14.5.

It does not work on Node.js 4.0 0.12 if I use version 1.16.0. Patch fix should be coming today.

brodybits pushed a commit that referenced this issue Sep 4, 2018
needed by Node.js pre-4.0

ref: #433
brodybits pushed a commit that referenced this issue Sep 4, 2018
needed by Node.js pre-4.0

ref: #433
@brodybits
Copy link
Collaborator

Should be fixed in recently published version 1.16.2.

@ljharb can you explain if there is any reason to continue supporting v1.x with support for Node.js pre-6.0? Is Node.js pre-4.0/pre-6.0 needed to test JavaScript shim? If so, would you guys be open to using something like Duktape to test shim of newer JavaScript versions?

It would be ideal if we can drop and abandon support for Node.js 6.0 (in new major release).

@ljharb
Copy link
Author

ljharb commented Sep 4, 2018

The shims need to be tested in older engines; duktape definitely wouldn't be feasible. It's totally fine if you want to drop support in v2; if it ever stops working, i'd have to migrate my tests off jasmine (i've already begun migrating all my packages that use mocha and jest off of them, for this reason). I'd rather not do that in this particular repo, though :-)

@ljharb
Copy link
Author

ljharb commented Sep 4, 2018

Thanks; the issue is indeed fixed https://travis-ci.org/es-shims/es5-shim/builds/424538191

Much obliged!!

@ljharb ljharb closed this as completed Sep 4, 2018
@brodybits
Copy link
Collaborator

Thanks for the confirmation. I've been thinking if es5-shim is one of the most important active users of this library then it may not make sense to drop Node.js pre-4.0 anytime soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants