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

Revisit dev watcher-logic #351

Closed
nickbalestra opened this issue Jan 13, 2017 · 18 comments
Closed

Revisit dev watcher-logic #351

nickbalestra opened this issue Jan 13, 2017 · 18 comments

Comments

@nickbalestra
Copy link
Contributor

As per #346 there will be the need to rework the dev watcher logic.
Now we watch on changes on 'server.js' but with the new compiler pipeline we open scenarios where changes might happen in a local module file and not directly in the main entry server file.

@nickbalestra
Copy link
Contributor Author

Perhaps we could simply rely on the watcher provided with webpack, that should not only solve the issue but due to its caching-logic should speed-up the bundling process.
https://webpack.github.io/docs/node.js-api.html#watching

@matteofigus
Copy link
Member

@nickbalestra tagging templates here as watching mechanism once templates are done could just be part of each template's configuration and in case this issue would fix.

@nickbalestra
Copy link
Contributor Author

nickbalestra commented May 11, 2017 via email

@debopamsengupta
Copy link
Member

debopamsengupta commented May 19, 2017

Hi guys, noticed you have this issue open and wanted to ask if there could be an improvement to the watcher to ignore changes in anything mentioned in .gitignore ?

I usually always end up with
Changes detected on file: /<path-to-component>/.git/FETCH_HEAD

Also tracking changes on a huge node_modules folder is very CPU intensive

@nickbalestra
Copy link
Contributor Author

Hey Dev, did you find any work-around/solution to this?

On a side I started doing some explorations on the watcher logic, and I think there are few small things we could quickly change to improve the overall DX.

  • Atm once a change is detected we repackage all the components in the dev registry directory, I think we could just repackage the affected component.
  • Once the packaging happen upon a change we could rely on a live reload logic to automatically refresh the preview page. This should shorten the feedback loop when dev quite a bit. What you recon?

@debopamsengupta
Copy link
Member

Hi,
That sounds good :) I can also take a look as to how we can potentially ignore certain files/folders (my thoughts are, mainly node_modules)
Right now, we usually disable the watcher logic if we are working with more than 1 component :(

@nickbalestra
Copy link
Contributor Author

nickbalestra commented Sep 8, 2017

This should address the first point -> #632. Perhaps it could help you in the scenario you describe where you have to disable the watcher if working on multiple components

@danrr
Copy link

danrr commented Sep 8, 2017

It would be nice to be able to define a white/blacklist for the watcher in, say, package.json or oc.json or as a CLI arg

@nickbalestra
Copy link
Contributor Author

Agree @danrr. I feel it could be a good thing to host that within the oc.json

@nickbalestra
Copy link
Contributor Author

I did some investigations for the ignore story, here my findings to confirm @debopamsengupta report.

  • It seems that we don't ignore from watching any file atm, we simply ignore some results of the watching process for the followings [node_modules, package.tar.gz, _package, .sw[op], .git, .DS_Store]

My question is, if we move those to be properly ignored and not added to the tree of files to be watched, will this be enough for you @debopamsengupta @danrr or will that still leave open cases where we actually need to add further ignores? My feeling is that this should solve the majority of the cases without the need of of an ignore configuration. at least for now

@danrr
Copy link

danrr commented Sep 8, 2017

It would make it nicer but there is still one big issue: project configuration folders for various IDEs like .idea or .vscode. It'd be a losing battle to try and ignore any possible IDE or VCS config by default...

@nickbalestra
Copy link
Contributor Author

fair point. The question is (as those file don't normally change that often and don't hide universes like node_modules and .git often do) if having a couple of such files added to the watch list will make such a big difference to the point we should really add a feature to ignore them. I'm try to think when/where those will make such a big problem in this context

@danrr
Copy link

danrr commented Sep 8, 2017

We have had a lot of complaints from people within Skyscanner about IDE config folders specifically. They can cause the registry to repackage at random times causing some frustration.

@nickbalestra
Copy link
Contributor Author

With #632 I think that shouldn't happen anymore as the repackaging will not happen for the whole registry anymore, and secondary only if a file within the component has changed.

I suggest we could first try to have another PR to fix what @debopamsengupta outlines in regard to .git and node_modules and see if that together with #632 silence those IDE issues. If not we can add a specific configuration in the oc.json to allow for custom ignores.

@nickbalestra
Copy link
Contributor Author

nickbalestra commented Sep 8, 2017

I also see we don't ignore dotFiles. @matteofigus any specific reasons for this? If we set this to true probably most of the iIDE config issues reported should be addressed. @danrr do you foresee any issue in ignoring dotfiles within components folders in general while watching?

@debopamsengupta
Copy link
Member

Unsure if we'd want to specify ignoring of dotfiles, I like the idea of either

  • ignoring everything in .gitignore
  • having a blacklist in oc.json

This was referenced Sep 9, 2017
@nickbalestra nickbalestra self-assigned this Sep 9, 2017
@nickbalestra
Copy link
Contributor Author

The changes seems to provide quite a fair amount of speed improvement on the watch logic now. Let us know, once we publish it, if it addresses all your issues as well.

livereload

@nickbalestra
Copy link
Contributor Author

@debopamsengupta published v0.41.4 that should address this. Closing this for now, in case of any further issues please just reopen this

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

No branches or pull requests

4 participants