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

Make it watch #63

Closed
wants to merge 12 commits into from
Closed

Make it watch #63

wants to merge 12 commits into from

Conversation

latentflip
Copy link
Collaborator

Depends on #62 and probably needs tests or something.

@wraithgar
Copy link
Contributor

Watchify definitely needs a default, and we may want to force it to false in non dev mode.

@latentflip
Copy link
Collaborator Author

Totally agree. It should never be on in prod.

Philip Roberts

On 19 Dec 2014, at 00:24, Michael Garvin notifications@github.com wrote:

Watchify definitely needs a default, and we may want to force it to false in non dev mode.


Reply to this email directly or view it on GitHub.

@faiq
Copy link

faiq commented Dec 27, 2014

@latentflip im working on solving a similar problem myself. I want to keep a cache of the bundle in development mode, so i dont have to wait forever for my page to reload. However, i was worried about the race condition that happens when you have a cached bundle and something is updated in the js/ folder, but before the bundle can be rebuilt and saved as the bundled cache the client requests the bundle. Does this mean the client will get an old version of the bundle?

@wraithgar
Copy link
Contributor

@faiq we solve a similar problem in moonboots-hapi with a little function @latentflip wrote https://github.com/wraithgar/moonboots_hapi/blob/master/lib/lock-async-function.js

@faiq
Copy link

faiq commented Jan 13, 2015

@wraithgar can you please point me to where you used it! I'm really confused 😞 on how I should be using it.

@wraithgar
Copy link
Contributor

Now that I look at your comment again it seems that doesn't solve your specific problem maybe

@lukekarrys
Copy link
Contributor

@latentflip This is great! And I really want it to land to save me from long build times in developmentMode. So I added a commit to address the following:

  • watchify has a default of false and always gets set to false if developmentMode is false
  • added watchify:true to the dev test and and added a test that requests the jsSource twice to make sure 1) it doesnt break and 2) it gets coverage up to 100% because then it can set bundle to self.watchedBundle.

Anything else that needs to get discussed for this? Should we go ahead and update to v8 of browserify now that its out?

@faiq
Copy link

faiq commented Jan 20, 2015

@latentflip, how did you solve the concerns I posted above, I would really love some guidance. :D

@lukekarrys
Copy link
Contributor

I pulled this branch into my current project using moonboots_hapi and encountered an error when using it with the modulesDir option (which uses browserify.require and the expose option).

It looks like the result of this bug with watchify: browserify/watchify#72.

I'm gonna come up with a failing test and then see if one of the workarounds in that issue will fix it.

@lukekarrys
Copy link
Contributor

I made a failing test where if you get jsSource() twice for a watchified bundle, the bundles are different and any file exposed as part of modulesDir wont be able to be required properly.

I tried updating browserify and watchify to see if any of the workarounds in the issue above worked, but none of them did.

Seems like the only way watchify could be included is if we detect both modulesDir and watchify in the config, we throw an error. Otherwise it'll lead to hard to track down bugs where users just get {} as the result of requiring a module. But that seems like not a great idea.

Keeping an eye on the related watchify issues:

@lukekarrys lukekarrys closed this Feb 15, 2015
@lukekarrys lukekarrys deleted the make-it-watch branch September 11, 2015 18:02
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