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.

Sign up for GitHub

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

Jump to bottom

[fixes #121] chokidar -> sane #1283

Merged
merged 1 commit into from
Jul 30, 2018
Merged

[fixes #121] chokidar -> sane #1283

merged 1 commit into from
Jul 30, 2018

Conversation

stefanpenner
Copy link
Contributor

@stefanpenner stefanpenner commented Jun 6, 2018 •
edited
Loading

I think chokidar is great, but the ergonomics around native dependencies today in node really painful. Although chokidar pre-builds, users switching nodes, or with old yarn.lock files, or failed pre-build downloads, or behind firewall usages cause a fallback to requiring a working compiler toolchain. Requiring that, very much increases the barrier of entry, and support load for tooling/cli maintainers.

@stefanpenner
Copy link
Contributor Author

stefanpenner commented Jun 6, 2018 •
edited
Loading

I'll try to land: amasad/sane#121 to make this slightly more compelling.

@trentmwillis
Copy link
Member

...cause a fallback to requiring a working compiler toolchain. Requiring that, very much increases the barrier of entry, and support load for tooling/cli maintainers.

What does this mean practically? Can you provide a concrete example of how this might increase the barrier of entry?

@stefanpenner
Copy link
Contributor Author

stefanpenner commented Jun 6, 2018 •
edited
Loading

What does this mean practically? Can you provide a concrete example of how this might increase the barrier of entry?

Totally fair, let me share my opinion.

prebuilt dependencies work only if:

  • access to AWS
  • prebuilt for your OS + Node ABI is available on AWS
  • you don't switch node versions.

If any of the above fall sideways, you end up requiring:

  • python / node-gyp + compiler toolchain (like gcc) working

This affects the following users:

  • unfamiliar with node, especially those on windows
  • with firewall rules (work rules) preventing aws cdn access
  • power-users which switch node versions often
  • people doing trainings, where their users have "issues with their environment". Pure JS deps by default, avoid a whole host of these issues.

trentmwillis
trentmwillis reviewed Jun 6, 2018
View reviewed changes
Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed response. I'm 👍 on this change given the above info.

Looks like all the --watch tests are currently failing in CI though. Once those are fixed I'm good to land this.

@stefanpenner
Copy link
Contributor Author

Looks like all the --watch tests are currently failing in CI though. Once those are fixed I'm good to land this.

👍

@stefanpenner
[fixes qunitjs#121] chokidar -> sane
1f0190e 
I think chokidar is great, but the ergonomics around native dependencies today in node really painful. Although chokidar pre-builds, users switching nodes, or failed pre-build downloads, or behind firewall usages cause a fallback to requiring a working compiler toolchain. Requiring that, very much increases the barrier of entry, and support load for tooling/cli maintainers
@stefanpenner
Copy link
Contributor Author

Sorry about the delay, just fixed the tests locally. Let's see what CI says...

trentmwillis
trentmwillis approved these changes Jul 30, 2018
View reviewed changes
Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks 😄

bin/run.js
} );

watcher.on( "ready", () => run.apply( null, args ) );
watcher.on( "change", watcherEvent( "changed", args ) );
watcher.on( "add", watcherEvent( "added", args ) );
watcher.on( "unlink", watcherEvent( "removed", args ) );
watcher.on( "delete", watcherEvent( "removed", args ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a personal note, I prefer this event name so much more than "unlink".

@trentmwillis trentmwillis merged commit 9af0448 into qunitjs:master Jul 30, 2018
@trentmwillis trentmwillis mentioned this pull request Jul 31, 2018
Closed
@stefanpenner stefanpenner deleted the no-native-deps branch July 31, 2018 14:28
@stefanpenner
Copy link
Contributor Author

@trentmwillis is there a release planned soon, or will it be some time? (I am unfamiliar with the QUnit release cycles)

@trentmwillis
Copy link
Member

We don't really have an official release cycle. It's pretty much after we've had some worthwhile changes and/or someone asks for it. I'll plan to do one this week.

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

@trentmwillis trentmwillis trentmwillis approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stefanpenner @trentmwillis