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 fs-extra to v8 #70

Merged
merged 1 commit into from Jun 11, 2019

Conversation

@dpogue
Copy link
Member

commented May 11, 2019

Platforms affected

All, but hopefully mostly Windows

Motivation and Context

Hopefully resolves #64 apache/cordova#121 on cordova-common only

Description

Updated fs-extra to the latest version (8.0.0).

Testing

npm test passed locally on Linux, will ensure it's green on CI.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)

@project-bot project-bot bot added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests May 11, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 11, 2019

Codecov Report

Merging #70 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #70   +/-   ##
=======================================
  Coverage   89.04%   89.04%           
=======================================
  Files          20       20           
  Lines        1826     1826           
  Branches      374      374           
=======================================
  Hits         1626     1626           
  Misses        200      200

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cd117e...c0cb95e. Read the comment docs.

@raphinesse

This comment has been minimized.

Copy link
Contributor

commented May 11, 2019

AFAICT, just updating will not fix anything. I think the new version just includes an opt-in workaround to disable some checks during copy*. The downside being that this could delete your copy source. And that we'd have to edit each and every location where we copy something.

Or is that information outdated?

@FlorinSerban

This comment has been minimized.

Copy link

commented May 13, 2019

@raphinesse

AFAICT, just updating will not fix anything. I think the new version just includes an opt-in workaround to disable some checks during copy*. The downside being that this could delete your copy source. And that we'd have to edit each and every location where we copy something.

Or is that information outdated?

I think the right fix was implemented. Please check this comment.

@raphinesse

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Looks like you're right, @FlorinSerban. Thanks for the pointer!

@catalinmitrofan

This comment has been minimized.

Copy link

commented May 13, 2019

When are you guys planning a release with this fix?

@oliversalzburg

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@catalinmitrofan It hasn't even been merged yet. Calm down 😁

@catalinmitrofan

This comment has been minimized.

Copy link

commented May 13, 2019

@oliversalzburg, I just want to know, when the changes will be merged => when is a new release planned?

@oliversalzburg

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

I believe there is no schedule. If this is a blocking issue for you, I recommend implementing a workaround for the time being.

@dimitriscsd

This comment has been minimized.

Copy link

commented May 13, 2019

Considering the severity of the issue (Everyone with a windows build server is unable to build their cordova project), I believe there should be a schedule asap.

The workaround is a nice band aid but a proper fix is always better.

@oliversalzburg

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@dimitriscsd We have waited a long time for fs-extra to fix their code. There is really no need for this kind of pressure so soon after a change has been proposed in Cordova. I'm sure everyone agrees that this needs to be fixed ASAP.

@dimitriscsd

This comment has been minimized.

Copy link

commented May 13, 2019

That's perfect. I didn't mean to apply pressure, I just pointed out that it's a serious problem users of Cordova are facing and it should be places somewhere in the pipeline rather than be left in a backlog somewhere.

It's great to hear that you guys understand the severity of the issue and I'm grateful for your work and efforts.

Apache Cordova: Tooling Pull Requests automation moved this from 🐣 New PR / Untriaged to ✅ Approved, waiting for Merge May 13, 2019

@brodybits

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Let's get this one merged and published. Thanks guys for the patience.

@dpogue

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Note that we also need to update fs-extra in any other modules that use it, which includes cordova-lib, cordova-create, and cordova-fetch, and possibly others

@catalinmitrofan

This comment has been minimized.

Copy link

commented May 13, 2019

Thanks again to everyone for their fast work on this issue.

@catalinmitrofan

This comment has been minimized.

Copy link

commented May 15, 2019

So, is there a planned release with this fix?

@brodybits

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

So, is there a planned release with this fix?

I suggest you post this kind of question to the forum at: dev@cordova.apache.org

I would like to clarify a couple things:

To fully "release" this fix, we would actually need to apply this fix and make a release for multiple components as @dpogue said in #70 (comment).

I personally do not have so much time to make a new release of the components right now. Keep in mind that this is a 100% volunteer project with very limited corporate sponsorship, if any. (I think a few big companies had donated maintainer time in the past, seems to be gone now.)

Thanks for your continued patience.

@catalinmitrofan

This comment has been minimized.

Copy link

commented May 16, 2019

@brodybits , thank you for the explanation and your answer. I will wait then for the next release

@l3ender

This comment has been minimized.

Copy link

commented Jun 7, 2019

Thanks for all those who have worked on this. Is there anything I could do to help out to get it merged and released?

package.json Show resolved Hide resolved
@brodybits brodybits referenced this pull request Jun 7, 2019
3 of 3 tasks complete
@brodybits

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

I would like to merge this, maybe with my fs-extra@^8.0.1 suggestion if there are no objections from other maintainers. Then one of us maintainers will have to publish a new cordova-common release in the next 1-2 weeks.

@l3ender and others please followup with us via email to dev@cordova.apache.org in case we drop this one. I think all maintainers are extremely busy in general.

@dpogue

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

As I understand it, the reason this has not been merged is because nobody has been able to actually confirm whether it fixes the problem or not (although it's very hard to test that when other libraries are also pulling in their own copies of fs-extra).

As long as we're confident that the major version bump of fs-extra doesn't break anything here, I'm good with seeing this merged.

@brodybits

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

I would personally favor publishing this change as a minor release; fs-extra@8 seems to not break for Node.js back to version 6. Any other comments or objections?

@catalinmitrofan

This comment has been minimized.

Copy link

commented Jun 7, 2019

@dpogue , I have moved the cordova-common repository to our own, and I patched it there. Since I patched it, I didn't get any issues when trying to build the app for windows. I have been using it like this for the last 20 days, without any issues. When the change will be published, then I will revert back to using the repository from here, until then, I am using my "patched" repository.

@brodybits brodybits self-assigned this Jun 11, 2019

@brodybits brodybits changed the title Update fs-extra to v8.0.0 Update fs-extra to v8 Jun 11, 2019

@brodybits brodybits merged commit 49a0d69 into apache:master Jun 11, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to 🏆 Merged, waiting for Release Jun 11, 2019

@brodybits

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

I have just merged this proposal, with an updated title since it will include latest patch and minor release on fs-extra@8. I will now propose a minor release to be published within the next week.

@brodybits brodybits referenced this pull request Jun 11, 2019
4 of 4 tasks complete
@brodybits

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

This update is now published in cordova-common@3.2.0. I documented the recommended update procedure in apache/cordova#121 (comment).

@gwynjudd

This comment has been minimized.

Copy link

commented Aug 20, 2019

I'm still getting this issue with the updated version of cordova-common and fs-extra. Things I've checked:

  1. version of cordova-common is 3.2.0
  2. version of fs-extra is 8.1.0

node version is 8.9.1

Any other things I should be checking?

@brodybits

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

It is more likely to work on Node.js 10 which supports bigint. There may be some more workarounds for older Node.js versions but I would consider it to be a waste of time.

@gwynjudd

This comment has been minimized.

Copy link

commented Aug 20, 2019

@brodybits thanks I'll give it a shot

EDIT: looks to have gotten it working with node v 10.16.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.