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

workarounds for browserify #52

Closed
wants to merge 2 commits into from

Conversation

danielepolencic
Copy link

fixes #24

  • if stderr is not available, inject the console in squeak
  • process.exit is not available in the browser. Don't call the method if it is not available.

@danielepolencic danielepolencic changed the title redirect output to console if stderr is not available workarounds for browserify Sep 16, 2015
@sindresorhus
Copy link
Member

I still think this is better fixed in squeak: #24 (comment)

@@ -83,7 +84,9 @@ function exit() {
stack(results);
}

process.exit(stats.failCount > 0 ? 1 : 0);
if (process.exit) {
Copy link
Member

Choose a reason for hiding this comment

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

How will this work with hihat? According to its docs:

The process will stay open until you call window.close()

Wouldn't it be better to call window.close here if in the browser. Or even better, keep it as before, and have hihat shim process.exit => Experience-Monks/hihat#27

@sindresorhus
Copy link
Member

// @mattdesl

@mattdesl
Copy link

Hmm.. window.close would also work with smokestack but it seems like a pretty big assumption.

In hihat the goal is interactive testing (fast save -> reload workflow). If ava exits the process on fail, it would break this development cycle. Maybe ava should not quit at all in the browser?

@sindresorhus
Copy link
Member

@mattdesl Good point. So what should we do here?

@Qix-
Copy link
Contributor

Qix- commented Sep 20, 2015

Hmm. Perhaps ava.on('exit', ...), where ava detects if it's in node and registers the default handler of process.exit()? Let test implementers decide; we can add sensible defaults as necessary.

@Qix-
Copy link
Contributor

Qix- commented Sep 20, 2015

Or even shim process.exit()? It fires the exit event (process.on('exit', ...)) so that might even be the ideal way anyway to keep full compatibility.

@vadimdemedes
Copy link
Contributor

Agree with @mattdesl, on client-side process.exit() should do nothing or send an exit event, but definitely not close a window.

@mattdesl
Copy link

It seems sorta weird that an API would ever call process.exit. I feel like this should be left up to the user.

I feel like only the CLI should exit and handle errors itself, since in that case the user isn't handling them.

@vadimdemedes
Copy link
Contributor

@mattdesl this happens, because we want to cover a case when test is being executed directly.

$ node test-something.js

So if only CLI would use process.exit(), in the above case process would exit always with zero (success).

@mattdesl
Copy link

I see. It's a bit of a special case then.

@arthurvr
Copy link

Or even shim process.exit()? It fires the exit event (process.on('exit', ...)) so that might even be the ideal way anyway to keep full compatibility.

To me that sounds the most reasonable way to me. Though I guess this is kind of a rare case anyways.

@Qix-
Copy link
Contributor

Qix- commented Sep 21, 2015

It is, but it's still probably the correct way. Just a thought.

@scottcorgan
Copy link

Just curious if there's been any progress for the browser. Only thing keeping me from Ava.

@sindresorhus
Copy link
Member

@kevva said he would make squeak browserifyable and this PR needs to be updated to only include the exit change.

@sindresorhus
Copy link
Member

Might be more, though, as we recently introduced parallism, which forks the processes. Not sure whether browserify handles that or if we need to do some special casing. Help welcome. Our main focus is elsewhere.

@kevva
Copy link
Contributor

kevva commented Oct 20, 2015

Browser support in squeak is fixed in kevva/squeak@fd7333e.

@kevva
Copy link
Contributor

kevva commented Oct 20, 2015

... this PR needs to be updated to only include the exit change.

So what should we decide on? Just check for process.exit and do nothing if it doesn't exist?

@sindresorhus
Copy link
Member

Closing in favor of #97.

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

Successfully merging this pull request may close these issues.

Browser support
8 participants