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

refactor(tools): use fs.promises API #2282

Merged
merged 2 commits into from
Apr 16, 2019
Merged

refactor(tools): use fs.promises API #2282

merged 2 commits into from
Apr 16, 2019

Conversation

saschanaz
Copy link
Collaborator

Uses fs.promises which is now marked as stable.

@@ -2,7 +2,7 @@

"use strict";
const colors = require("colors");
const fsp = require("fs-extra");
Copy link
Member

Choose a reason for hiding this comment

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

We can remove fs-extra from package.json now, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

copydeps still uses fs-extra for its own utility functions, but maybe we could cook our own ones.

Copy link
Member

Choose a reason for hiding this comment

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

I checked. cp is being used only for files (not directories), so we can use the native one easily. And we have fs.rmdir and fs.unlink for fsp.remove 🎉

Copy link
Member

Choose a reason for hiding this comment

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

@saschanaz Do you plan to fix above here or separate PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, maybe a separate one.

@sidvishnoi
Copy link
Member

Should we update this too?
https://github.com/w3c/respec/blob/2c073773d343e3a93196ad20307d5508a3df83d0/package.json#L7

@saschanaz
Copy link
Collaborator Author

saschanaz commented Apr 16, 2019

Should we update this too?

Ah yes... but should we defer this until version 8 loses its support? Not sure.

@sidvishnoi
Copy link
Member

fs.promises isn't available in node 8, right? It'll break if use node 11 features there

@saschanaz
Copy link
Collaborator Author

The API is introduced in Node.js 10, but yes, it will break 8.

Copy link
Contributor

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

This is in node lts, right?

@saschanaz
Copy link
Collaborator Author

This is in node lts, right?

Sure 😃

@marcoscaceres
Copy link
Contributor

We should target active lts only. Ok to delete 8.

@saschanaz
Copy link
Collaborator Author

One notable thing is that nodejs/node#26581 is not backported into 10 LTS yet, so using ReSpec there may emit some warnings.

@saschanaz
Copy link
Collaborator Author

saschanaz commented Apr 16, 2019

is not backported into 10 LTS yet, so using ReSpec there may emit some warnings.

Like this... Looks like we should wait a bit more 😅

6:50:21 AM  ⏲  Running ReSpec2html tests...
6:50:21 AM  👷‍♀️  node ./tools/respec2html.js -e --disable-sandbox --timeout 30 --src http://localhost:5000/examples/basic.built.html --out /dev/null (test 1/2)
(node:4775) ExperimentalWarning: The fs.promises API is experimental
6:50:22 AM  👷‍♀️  node ./tools/respec2html.js -e --disable-sandbox --timeout 30 --src http://localhost:5000/examples/basic.html --out /dev/null (test 2/2)
(node:4838) ExperimentalWarning: The fs.promises API is experimental
The command "npm run test:headless" exited with 0.

@sidvishnoi
Copy link
Member

Lets pipe it through grep -v "ExperimentalWarning: The fs.promises API is experimental" 😛

@marcoscaceres
Copy link
Contributor

The warnings are ok IMO. Reading the history, the API did not change from when it was experimental to when it became stable... so technically, it was stable. Also, the warning don’t affect processing, so I think it’s fine to ship.

@saschanaz saschanaz merged commit 5d2d21a into develop Apr 16, 2019
@saschanaz saschanaz deleted the fs-promises branch April 16, 2019 11:05
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.

3 participants