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

[WIP] Browser support #887

Closed
wants to merge 8 commits into from
Closed

[WIP] Browser support #887

wants to merge 8 commits into from

Conversation

vadimdemedes
Copy link
Contributor

@vadimdemedes vadimdemedes commented May 28, 2016

Check out the commit messages, I tried to make them as descriptive as possible.

Implementation and usage details, vision: #24 (comment).

Coming soon.

Running AVA in browser environment won't allow
API to execute file system operations, so
file searching (globbing) needs to be extracted.

After this change, API expects a set of already
resolved, absolute paths.
avaFiles = AvaFiles;

avaFiles.prototype.findTestFilesSync = function () {
return this.files;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because API expects resolved paths, findTestFilesSync needs to be stubbed to return "input" files without changes, in order to easily check them in tests. Otherwise, findTestFilesSync would return empty array.

@jamestalmage
Copy link
Contributor

@vdemedes - mind sharing where you are going with this? There are lots of different ways to attack browser support, and we probably should all agree on the best way.

For instance, in my mind api.js was never going to run browser side. We would have done that with an alternate to fork.js that launched browsers and then received events via socket.io.

@novemberborn novemberborn mentioned this pull request May 29, 2016
@novemberborn
Copy link
Member

I agree with @jamestalmage here. Let's continue the discussion in #24 (comment)?

vdemedes added 7 commits May 29, 2016 17:47
Note, this is an internal flag and is not supposed
to be exposed to end users. It's used only when
test files were previously precompiled, to avoid
double compilation and caching.
When `globals.setTimeout` or `globals.clearTimeout`
is called with `globals` context, browser (at least
Chrome) throws "Illegal Invocation" error. If those
functions are called with `null` (global context),
errors are not being thrown.
@vadimdemedes
Copy link
Contributor Author

I pushed browser support work I've done, so that you can get a glance.

To test it out:

  1. Generate tests.
$ ava --browser
  1. Start http server to serve current directory (I use serve):
$ serve
  1. Open http://localhost:3000 in your browser.
  2. Open console.

@vadimdemedes
Copy link
Contributor Author

Implementation and usage details: #24 (comment).

return api.run([path.join(__dirname, 'fixture/ignored-dirs/fixtures/test.js')]);
});

test(prefix + 'test file in helpers is ignored', function (t) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lots of tests being deleted here. Many of which guard specific regressions. If the intent is to reimplement later, I would prefer skipping instead:

test('tilte', { skip: true }, function (t) {
  ...
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those tests are not deleted, they moved to ava-files.js tests, which covers these cases.

@jamestalmage
Copy link
Contributor

Some thoughts after an initial review:

I don't think we should try to run the API in the browser. Instead, we should just be reimplementing lib/send.js to send messages via socket.io - the API would continue to run in Node.

There is also already a whole lot to review for a single PR. I would like to see an incremental approach.

I think the first step should be creating a build/bundle tool that packages a single test with browser safe versions of all the child process dependencies. I don't think socket.io interop would even need to be tackled in that first batch, we could just console.log messages sent to the parent (profile.js already contains polyfills for process.send that does just that).

@vadimdemedes
Copy link
Contributor Author

I don't think we should try to run the API in the browser.

Why? I had no issue with doing that.

@@ -479,18 +480,6 @@ function generateTests(prefix, apiCreator) {
});
});

test(prefix + 'search directories recursively for files', function (t) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see this test replicated anywhere.

@jamestalmage
Copy link
Contributor

I don't think we should try to run the API in the browser.
Why?

We have efforts underway to bring watcher into the API, and simplify cli.js so it's a simple wrapper around it.

The less complexity we push into the browser, the better.

I had no issue with doing that.

It's not so much a matter of whether or not it's possible, but rather if it is a good idea. It's definitely adding complexity.

I think it would be better if we started trying to make a browser fork that behaves like a child_process fork, replacing node's child process ipc, with socket.io stuff.

@vadimdemedes
Copy link
Contributor Author

We have efforts underway to bring watcher into the API

Is there a discussion/issue/pr that I missed?

It's not so much a matter of whether or not it's possible, but rather if it is a good idea. It's definitely adding complexity.

I don't see how it adds more complexity. I'm not modifying API in any way, it's good to work in the browser 1:1, unchanged. As I mentioned previously, I was trying to reuse as much as possible. I don't want to rewrite AVA core for browsers. Instead, the plan was to replace node-dependent parts with browser-compatible ones. What makes it a bad idea? Do you have specific arguments against doing this?

@jamestalmage
Copy link
Contributor

Is there a discussion/issue/pr that I missed?

#533 (comment)
#759 (comment)
#865

I don't want to rewrite AVA core for browsers

Not suggesting that. I'm suggesting you rewrite less than you have.

Instead, the plan was to replace node-dependent parts with browser-compatible ones

I do not disagree with this. I just think we could get there simpler if we sent stuff over the wire to the console instead of trying to do stuff in the browser.

@niieani
Copy link

niieani commented Aug 24, 2016

What's the status on this?

@novemberborn
Copy link
Member

Closing this since it's been languishing. Let's hash out our desired implementation in #24.

@novemberborn novemberborn deleted the browser-support branch July 13, 2017 10:12
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.

4 participants