Skip to content

replace readFileSync #212

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 16, 2020
Merged

replace readFileSync #212

merged 1 commit into from
Apr 16, 2020

Conversation

Yanpas
Copy link
Contributor

@Yanpas Yanpas commented Apr 14, 2020 •
edited
Loading

I decided to do it sequentially to keep the same behaviour and predictable memory consumption.

@Yanpas Yanpas mentioned this pull request Apr 14, 2020
Closed
skovhus
skovhus approved these changes Apr 15, 2020
View reviewed changes
Copy link
Collaborator

@skovhus skovhus left a comment

Choose a reason for hiding this comment

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

Thanks!

@skovhus
Copy link
Collaborator

skovhus commented Apr 16, 2020

I realize now that using the fs.promises API means that we limit the support to node 10+.

So either we should:

  1. Update the required node version here https://github.com/bash-lsp/bash-language-server/blob/master/server/package.json#L18
  2. Use the util.promisify() API (node 8) as you originally suggested

Thoughts?

@Yanpas
Copy link
Contributor Author

Yanpas commented Apr 16, 2020

I think the second one is better. Ubuntu 18.04 ships 8th lts version e.g.

@skovhus
Copy link
Collaborator

skovhus commented Apr 16, 2020

Let us go with 2 then. 👍 I’ll update CI to test on node 8.

@skovhus skovhus mentioned this pull request Apr 16, 2020
Merged
@skovhus skovhus merged commit 4a1ae50 into bash-lsp:master Apr 16, 2020
@skovhus
Copy link
Collaborator

skovhus commented Apr 16, 2020

Replaced by #213

@Yanpas Yanpas deleted the async_fileread branch April 16, 2020 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skovhus skovhus skovhus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Yanpas @skovhus