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

Use browserify cache when bundling #844

Closed

Conversation

carlos-granados
Copy link

This work is heavily inspired by uber-watchify https://github.com/benbria/uber-watchify which I found I could not use directly, so I used a lot of its code as inspiration

Browserify provides the possibility of using a cache which greatly speeds the generation of the bundled files. This cache is for example used if we use watchify and re-bundle a file which has been updated. But this cache is not persisted so it is not used when we start tests from scratch. If the tests to bundle are complicated, with a lot of common files, this bundling of all tests can take a long time (was taking almost a minute in our case when bundling the whole test suite to run headlessly)

We can take advantage of this cache by saving it to a file once the target has been bundled and reading it from this file when we need to bundle the target again. By doing this, bundling our whole test suite was down to less that 10 seconds

There are a couple of things that we need to add to make this work:

  • When using the cache, we need to make sure that any cached files have not been updated. We do this by comparing the timestamp of the cached file to the timestamp of the cache
  • If browserify reads a file from the cache instead of the disk, it will not emit a 'file' event (as the file is not actually read) and watchify will not watch this file. We need to manually emit a 'file' event for any file that is read from the cache so that watchify watches it

Apart from that I fixed that even if the watchForFileChanges setting was false, it would still activate watchify

Includes unit tests. I was not really sure how to add integration tests or e2e tests so it does not include them

@brian-mann
Copy link
Member

Solid. @chrisbreiding will take a look at this since he's written all of the browserify stuff.

I conceptually understand how this works and it makes sense.

The only thing that initially sticks out at me is the usage of fs.sync. Unfortunately, we cannot ever use any synchronous file reading / writing at Cypress because it has shown to break on a significant number of user machines due to EMFILE errors.

Those calls will need to be async.

@carlos-granados
Copy link
Author

carlos-granados commented Oct 28, 2017

I changed the code so that the cache file is now saved asynchronously. Not sure if this is needed also for the calls to the statAsync method as making this asynchronous would complicate the code quite a bit

@chrisbreiding
Copy link
Contributor

This looks great. The only problem is that the browserify code is being extracted into its own repo as part of the plugins/preprocessors API. It's pretty close to being finished and merged, so even if we merged this PR, it would be pretty short-lived.

Would you mind creating a PR against the browserify preprocessor repo? To get it running, you'll need to checkout the preprocessor-refactor branch of this repo and link your fork of the browserify repo.

Also, I know it's a pain, but the statSync call will need to be made asynchronous as well. Pretty much any fs usage needs to be async or there's a risk of crashing due to an EMFILE error.

@carlos-granados
Copy link
Author

Ok, will move this to the browserify preprocessor repo and look into making the stat call synchronous

@brian-mann
Copy link
Member

@carlos-granados now that the plugins API has landed and finalized - to get this merged in you can just submit a PR against here: https://github.com/cypress-io/cypress-browserify-preprocessor

Also check out our new docs on the plugins APIs - they go into a lot of detail about how it all works together. Since its now in a separate repo it could even be merged in earlier and then used by you and your team even before it lands in Cypress itself.

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.

None yet

4 participants