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

[Feature] Automatically detect safe-write #221

Closed
edo-codes opened this issue Dec 11, 2017 · 46 comments
Closed

[Feature] Automatically detect safe-write #221

edo-codes opened this issue Dec 11, 2017 · 46 comments
Labels
HMR Hot Module Reloading 🙋‍♀️ Feature
Projects

Comments

@edo-codes
Copy link

edo-codes commented Dec 11, 2017

🐛 Bug report

While the parcel dev server or the watcher is running, changes in a file are sent to the browser correctly, but only the first time I save changes in the file. The second time, parcel doesn't rebuild (there is no console output) and the module doesn't reload in the browser. Other files will still reload, but also only once.

Even when refreshing in the browser, the changes aren't shown. I have to restart parcel for it to show the latest changes again.

Reproduction

Here's a minimal sample that shows the problem for me: https://github.com/Edootjuh/parcelrepro

To reproduce:

  • Start parcel with parcel index.html
  • Load http://localhost:1234 in browser. The page shows a b
  • Edit module-a and save. The page changes. Edit it again. The page doesn't change.
  • Edit module-b and save. The page changes. etc.

The same happens with parcel watch index.html

🌍 Your Environment

Software Version(s)
Parcel Latest on master (commit 2d680c0)
Node node v8.9.1
npm/Yarn yarn 1.2.1
Operating System Ubuntu 17.10
@davidnagli davidnagli added the HMR Hot Module Reloading label Dec 11, 2017
@davidnagli
Copy link
Contributor

I was able to reproduce this on Mac OS as well. However, the problem goes away when I remove module.hot.accept

@davidnagli davidnagli added this to To Do in Bugs via automation Dec 11, 2017
@edo-codes
Copy link
Author

That's weird. I just tried without module.hot.accept, and the problem is still there for me. (I've cleared the cache)

@davidnagli
Copy link
Contributor

Can you commit the new code (the code without module.hot.accept) to a new branch in your example repo?

@edo-codes
Copy link
Author

I'd just changed the code in the example repo to remove module.hot.accept in the master branch before your first comment, so if you pull/reset again it should update

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Dec 11, 2017

I can't seem to reproduce this in any way

@edo-codes
Copy link
Author

edo-codes commented Dec 11, 2017

I've done some poking around, and it seems like chokidar isn't firing change events for the subsequent edits in Bundler.js, but it does when I set usePolling to true. Obviously that isn't a fix, since that option results in poorer performance, but maybe that helps?

diff --git a/src/Bundler.js b/src/Bundler.js
index bb21004..cd0cf1f 100644
--- a/src/Bundler.js
+++ b/src/Bundler.js
@@ -176,7 +176,8 @@ class Bundler extends EventEmitter {
       // FS events on macOS are flakey in the tests, which write lots of files very quickly
       // See https://github.com/paulmillr/chokidar/issues/612
       this.watcher = new FSWatcher({
-        useFsEvents: process.env.NODE_ENV !== 'test'
+        useFsEvents: process.env.NODE_ENV !== 'test',
+        usePolling: true
       });
 
       this.watcher.on('change', this.onChange.bind(this));

@edo-codes
Copy link
Author

edo-codes commented Dec 11, 2017

I've found the problem. It has to do with the way many text editors write files, and is actually addressed by the webpack documentation: https://webpack.github.io/docs/webpack-dev-server.html#working-with-editors-ides-supporting-safe-write. To quote:

Working with editors/IDEs supporting “safe write”

Note that many editors support “safe write” feature and have it enabled by default, which makes dev server unable to watch files correctly. “Safe write” means changes are not written directly to original file but to temporary one instead, which is renamed and replaces original file when save operation is completed successfully. This behaviour causes file watcher to lose the track because the original file is removed. In order to prevent this issue, you have to disable “safe write” feature in your editor.

  • VIM - set :set backupcopy=yes (see documentation)
  • IntelliJ - Settings ▶︎ System Settings ▶︎ Synchronization ▶︎ disable safe write (may differ in various IntelliJ IDEs, but you can still use the search feature)

I suppose the reason I'd never had this problem before is because I'd always configured webpack to watch the directory instead of individual files.

This means this isn't really a parcel bug, but it is likely an issue that other people will face as well (I've tried editing the file with IntelliJ IDEA, vim and gedit, and they all behaved the same), so it might be worth adding an extra recursive watcher for e.g. the root directory so you can receive events for these 'new' files and rebuild.

@davidnagli
Copy link
Contributor

Wow thanks for hunting this down! I would have never figured that out.

Do you guys think we should add a warning about this in the documentation?

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Dec 11, 2017

We should maybe add an option to the cli to enable polling for the people who have this activated in their IDE? (because this isn't really an option as standard for performance reasons)
And document it

@edo-codes
Copy link
Author

Personally, I think watching the directory would solve it completely (though I could be wrong), but in the meantime adding a warning and a cli flag for usePolling seem like good options to me.

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Dec 11, 2017

isn't use polling poking the files every now and then, you might be right though?

@davidnagli
Copy link
Contributor

Is there a way to detect “safe write”? Maybe we can dynamically fallback to polling when we detect it.

@edo-codes
Copy link
Author

When you look at the raw fs events from chokidar, at least on Linux, on a safe-written file change you get a change event, and then two rename events (as opposed to a single change event).

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Dec 13, 2017

@Edootjuh Have you tried listening for the 'add' event, this would trigger if a new file gets written what happens with safe write i guess?
I can't unfortunately test any of this myself due to using mac OS, would be cool to have a fix in place that doesn't require any additional configuration or performance issues
Edit: this didn't work

@DeMoorJasper DeMoorJasper changed the title HMR only reloads once for each file 🐛 HMR only reloads once for each file Dec 13, 2017
@juniovitorino
Copy link

juniovitorino commented Dec 18, 2017

Here using VSCode the same behavior happen, VSCode + OS X High Sierra, HRM only works once.

@DeMoorJasper
Copy link
Member

This is an IDE related issue @juniovitorino turn off safe-write and it should work

@frhagn
Copy link

frhagn commented Oct 23, 2018

I'm also having problems with safe-write when using Visual Studio 2017 combined with parcel watch.
The problem with Visual Studio is that safe-write can't be turned of..

From what I can tell the safe-write process in Visual Studio looks like this when saving a file (file.js in this example):

  1. Create an empty temp file. 5jrvappx.wht~
  2. Write the content to the temp file. 5jrvappx.wht~
  3. Create a second empty temp file. file.js~RF46beda4b.TMP
  4. Delete the second temp file. file.js~RF46beda4b.TMP
  5. Rename the original file to the same filename as in step 3. file.js > file.js~RF46beda4b.TMP
  6. Rename the first temp file from to the original filename. 5jrvappx.wht~ > file.js
  7. Delete the temp file from step 5. file.js~RF46beda4b.TMP

@DeMoorJasper @kasbah maybe this will break the test:

   const temp = __dirname + '/input/5jrvappx.wht~';
   const file = __dirname + '/input/local.js';
   const backup = __dirname + '/input/local.js~RF46beda4b.TMP';
   function simulateSafewrite(content) {
      fs.writeFileSync(temp, content);
      fs.renameSync(file, backup);
      fs.renameSync(temp, file);
      fs.unlinkSync(backup);
    }

@mitar
Copy link

mitar commented Feb 8, 2019

I would also add that coming from Meteor which works well with editors which use safe write, this seems like something which should be fixable and it should not require disabling safe write, which is at most a workaround and cannot be seen as simply a solution.

@mitar
Copy link

mitar commented Feb 8, 2019

See here relevant code in Meteor.

@mitar
Copy link

mitar commented Feb 8, 2019

Some related issues:

It seems chokidar has option atomic which we could try to make it work correctly when it is set to true: https://github.com/paulmillr/chokidar#errors I have tested it and it seems it does not work.

So it seems we should try to fix chokidar here, not parcel.

@mitar
Copy link

mitar commented Feb 8, 2019

I think I fixed this. If you want to test it, just install git://github.com/mitar/chokidar.git in your app instead of the original chokidar.

See paulmillr/chokidar#791 for more details.

@DeMoorJasper
Copy link
Member

@mitar awesome work, hope it'll get merged into chokidar 🎉

@mikolajpp
Copy link

@mitar I have tried to install chokidar from your repo, as a fix for #2661, but it unfortunately does not work, at least in neovim. Only the first write is detected, afterwards reloading does not work.

@mitar
Copy link

mitar commented Mar 4, 2019

Strange. It works great for me for PyCharm. Make sure you really use that forked version and that it npm does not restore to previous version.

So I added to my package.json:

  "devDependencies": {
    "chokidar": "git://github.com/mitar/chokidar.git"
  }

And it works then.

@mikolajpp
Copy link

mikolajpp commented Mar 5, 2019

I have copied your entry verbatim.
Now my package.json looks like this:

{
  "dependencies": {
    "parcel-bundler": "^1.11.0"
  },
  "devDependencies": {
    "[chokidar](https://npmjs.com/package/chokidar)": "git://github.com/mitar/c$
    "elm-hot": "^1.0.1",
    "node-elm-compiler": "^5.0.3"
  }
}

Still, hot reload works only on the first edit in neovim. Afterwards it is broken.

@mitar
Copy link

mitar commented Mar 5, 2019

Sorry. Not sure why there was that Markdown in JSON. I edited my comment before accordingly.

@mikolajpp
Copy link

Ah right. Still, I have fixed this, removed node_modules, did a reinstall and it is still broken.
Is there any way I can verify that the modified chokidar is indeed used/run by parcel?

@mikolajpp
Copy link

I have installed chokidar-cli, which I presume uses the version from your repo, and running
it from the command line reproduces the same behaviour in neovim. First write is detected, subsequent writes are not.

@mitar
Copy link

mitar commented Mar 7, 2019

My PR has been merged into upstream chokidar.

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Mar 8, 2019

Should be fixed in Chokidar, closing this issue.

Feel free to open a new issue if this persists. (Do note that this fix has not been released yet and might take some time before your node_modules are up to date with the chokidar fix.)

Bugs automation moved this from To Do to Done Mar 8, 2019
@mitar
Copy link

mitar commented Mar 8, 2019

Shouldn't then this issue be closed once chokidar is released with that fix and parcel updates its package.json to point to it?

Also, before closing this issue, documentation for parcel should be updated to not advise anymore to disable safe write.

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Mar 8, 2019

@mitar if it’s a minor release the package.json shouldn’t be updated although we probably will. But docs should probably wait untill chokidar had the update.

Sent with GitHawk

@cmcaine
Copy link

cmcaine commented May 28, 2019

chokidar reverted @mitar's workaround because it broke another tool and this is broken again.

paulmillr/chokidar@b79120b

My setup:

$ yarn list parcel chokidar
yarn list v1.16.0
warning Filtering by arguments is deprecated. Please use the pattern option instead.
├─ chokidar@2.1.6
└─ parcel@1.12.3
Done in 0.74s.

Writing a tiny chokidar script shows that chokidar isn't seeing the changes when it's watching particular files:

const chokidar = require('chokidar')

chokidar.watch('src/content.ts').on('change', console.log)

Chokidar sees the first change to the file, but not the second and subsequent. Watching the directory instead fixes this. I think parcel should watch the whole directory and filter names on linux. This gives a quick fix that won't break any other tools.

@cmcaine
Copy link

cmcaine commented May 28, 2019

Obviously another fix would be to just pin chokidar at 2.1.3 or 2.1.4 for now.

I'm doing that locally with:

// package.json
  "resolutions": {
    "parcel/**/chokidar": "2.1.3"
  }

Which is a yarn feature: https://yarnpkg.com/en/docs/selective-version-resolutions

@Augustin82
Copy link

Obviously another fix would be to just pin chokidar at 2.1.3 or 2.1.4 for now.

I'm doing that locally with:

// package.json
  "resolutions": {
    "parcel/**/chokidar": "2.1.3"
  }

Which is a yarn feature: https://yarnpkg.com/en/docs/selective-version-resolutions

This fixed it for me, many thanks.

@gk9848970
Copy link

I was able to reproduce this on Mac OS as well. However, the problem goes away when I remove module.hot.accept

How and from where to remove it?

@gk9848970
Copy link

I've found the problem. It has to do with the way many text editors write files, and is actually addressed by the webpack documentation: https://webpack.github.io/docs/webpack-dev-server.html#working-with-editors-ides-supporting-safe-write. To quote:

Working with editors/IDEs supporting “safe write”

Note that many editors support “safe write” feature and have it enabled by default, which makes dev server unable to watch files correctly. “Safe write” means changes are not written directly to original file but to temporary one instead, which is renamed and replaces original file when save operation is completed successfully. This behaviour causes file watcher to lose the track because the original file is removed. In order to prevent this issue, you have to disable “safe write” feature in your editor.

  • VIM - set :set backupcopy=yes (see documentation)
  • IntelliJ - Settings ▶︎ System Settings ▶︎ Synchronization ▶︎ disable safe write (may differ in various IntelliJ IDEs, but you can still use the search feature)

I suppose the reason I'd never had this problem before is because I'd always configured webpack to watch the directory instead of individual files.

This means this isn't really a parcel bug, but it is likely an issue that other people will face as well (I've tried editing the file with IntelliJ IDEA, vim and gedit, and they all behaved the same), so it might be worth adding an extra recursive watcher for e.g. the root directory so you can receive events for these 'new' files and rebuild.

How to do this in VS Code?

@piyushk
Copy link

piyushk commented Jun 29, 2021

chokidar reverted @mitar's workaround because it broke another tool and this is broken again.

paulmillr/chokidar@b79120b

Should this issue be reopened since the fix in chokidar was reverted?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HMR Hot Module Reloading 🙋‍♀️ Feature
Projects
No open projects
Bugs
  
Done
Development

Successfully merging a pull request may close this issue.