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

Update file timestamps to avoid redundant re-syncs (Fixes #22) #23

Merged
merged 7 commits into from
Jan 28, 2019

Conversation

danielgindi
Copy link
Contributor

No description provided.

@danielgindi danielgindi changed the title Update file timestamps to avoid redundant re-syncs (Closes #22) Update file timestamps to avoid redundant re-syncs (Fixes #22) Jan 26, 2019
@stefanpenner
Copy link
Owner

Could you provide a failing test?

@danielgindi
Copy link
Contributor Author

@stefanpenner Yes I've added a test for that

@danielgindi
Copy link
Contributor Author

Note that for folders the mtime will change as files are added to the folders.
So there's not much point there, unless you will add an mtime update after the files are created/deleted there.
But that will be both overly complicated and kind of idiotic, because the folder's mtime will not really reflect the truth. If there's a sync, and some files have changed - ignoring some other files, the directories are different and have the same mtime - not a good idea.

@danielgindi
Copy link
Contributor Author

I don't really get why that test fails on some machines. Is utimes not working on some of those envs?

@danielgindi
Copy link
Contributor Author

Okay so this is a failure in very old nodejs versions:
nodejs/node#2069

You are targetting versions below node 6, which are not even supported anymore.
If you'll upgrade the testing envs, you'll also get the benefit of being able to upgrade the codebase to more modern syntax :-)

@stefanpenner
Copy link
Owner

Ya, I’m down with a major version bump. So feel free to bump the engine support to >= 8 (as node 6 is on its way out)

@stefanpenner
Copy link
Owner

(I’m also fine with es syntax/feature upgrade if you are interested)

@danielgindi
Copy link
Contributor Author

Should be good now

@danielgindi
Copy link
Contributor Author

@stefanpenner This should also be a major version bump

@danielgindi
Copy link
Contributor Author

@stefanpenner Is there an eta on this?

@stefanpenner
Copy link
Owner

@stefanpenner Is there an eta on this?

I'm hoping to review today/tomorrow.

Copy link
Owner

@stefanpenner stefanpenner left a comment

Choose a reason for hiding this comment

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

2 small adjustments and this should be good to ship. Thank you.

.travis.yml Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@danielgindi
Copy link
Contributor Author

@stefanpenner Done. It won't automatically resolve the "requested changes" as one of them is on a different file.

@stefanpenner stefanpenner merged commit 1d5fa70 into stefanpenner:master Jan 28, 2019
@stefanpenner
Copy link
Owner

Tests appear to be failing on OSX

 1) TreeSync fixtures/one nothing -> populated validate mtime stays the same:

      AssertionError: expected 1532724030000 to equal 1532724030412
      + expected - actual

      -1532724030000
      +1532724030412

      at Context.<anonymous> (tests/index.js:63:34)

@stefanpenner
Copy link
Owner

NM is was an old 8.x that was failing, latest 8.15 works fine.

@danielgindi
Copy link
Contributor Author

I have just tested it now:


  TreeSync
    fixtures/one
      nothing -> populated
        ✓ file content
        ✓ validate mtime stays the same
      existing empty directory -> populated
        ✓ file content
      rmdir operation is sync
        ✓ immediately reflects deletions
      input(same) -> input(same)
        ✓ has stable output (mtime, size, mode, relativePath)
      .sync
        ✓ returns a list of changed files
      input(same) -> input(same + newFile)
        ✓ has stable output (mtime, size, mode, relativePath)
      output populated -> input with a changed file
        ✓ should update changed files on initial build
      input(same) -> input(same - file)
        ✓ has stable output (mtime, size, mode, relativePath)
      validate walk-sync options
        ✓ should ignore files/folders it is told to ignore
        ✓ should only include globs it is told to include

  eslint
    ✓ should have no errors in tests/index.js (359ms)
    ✓ should have no errors in tests/linting.js
    ✓ should have no errors in index.js (74ms)


  14 passing (649ms)

on macOS 10.14.12.

What is your env exactly?

@danielgindi
Copy link
Contributor Author

NM is was an old 8.x that was failing, latest 8.15 works fine.

Oh. That's why I wanted to specifically go with the LTS version, not just 8.x. Just remembered I was looking at the specific version where LTS started...
But the chance of someone using a non LTS version of node 8.x is very small.

@stefanpenner
Copy link
Owner

some more cleanup -> #24
will release a major shortly

@stefanpenner
Copy link
Owner

released as v2.0.0 🎉

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.

2 participants