-
Notifications
You must be signed in to change notification settings - Fork 12
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
Update npm dependencies #38
Conversation
package.json
Outdated
@@ -41,16 +41,16 @@ | |||
"dependencies": { | |||
"brout": "^1.3.0", | |||
"listen": "^1.0.0", | |||
"mocha": "^8.4.0", | |||
"mocha": "^9.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you planning to have this land in Mochify as well in the long run? Even if the only relevant breaking change seems to be dropping Node 10 I'm wonderig if this would mean we'd have to do major version bumps on both mocaccino and Mochify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression that we're stuck with mocha 8 because mocha 9 cannot be browserified (whole reason for the rewrite that I'm not getting over the finish line 🤭). Did that change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My memories might be off here, but from what I can recall that move from Browserify to Rollup in Mocha (which you might be referring to?) happened in some version before 8 already and was covered here #35 (comment) and here mantoni/mochify.js#213 (comment) so I think Mocha 9 should work. That doesn't necessarily make it true though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR seems to build fine when brought into mochify.js: mantoni/mochify.js#276
@mantoni Are you referring to being stuck on an old version of browserify instead of mocha (mantoni/mochify.js#224)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used to break, I've sent a PR to fix it, but nobody replied. mochajs/mocha#4121
This was the whole reason I went off to write a new mochify, that isn't depending on a bundler (plus some other improvements in the design). See https://github.com/mantoni/mochify.js/milestone/1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your PR says
I'm maintaining mochify and would like to move away from depending on the pre-bundled Mocha.
which also aligns with what I remember: it's just not possible to browserify browser-entry.js
file anymore because of that missing transform. Mocaccino however currently loads the pre-bundled mocha/mocha.js
here:
Lines 65 to 67 in aee84ab
var mochaPath = opts.mochaPath | |
? resolve.sync('mocha.js', { paths: [opts.mochaPath] }) | |
: resolve.sync('mocha/mocha'); |
so I think Mocha 9 should work fine as long as we consume the bundled version.
That being said, if I can help with finishing that rewrite in some way, I am happy to help, it will be worth it nonetheless :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should we update to mocha 9 and leave a bundled mocha.js or should we stay at mocha 8 for consistency until the rewrite finishes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I trust you with this decision. If you have time to do the work, I'll support the upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the mocha update from this PR. It can be added in a later PR.
d2544af
to
38acd8e
Compare
Now that #37 is landed, I rebased this PR and it should be ready to land, pending the mocha question. |
Shall I merge and release a major? |
package.json
Outdated
}, | ||
"devDependencies": { | ||
"@studio/changes": "^2.2.0", | ||
"browserify": "^16.2.3", | ||
"browserify": "^17.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should stick to 16 here so that it's in sync with the (blocked) Mochify: mantoni/mochify.js#224
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the browserify update
38acd8e
to
acfab5b
Compare
acfab5b
to
4346fd7
Compare
Thank you 🙌 if you're ready for upgrading Mocha too, let's do this. |
Depends on #37
There are still a few dependencies not updated: