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

Added TextDecoder require to fix node version 10.x #154

Merged
merged 2 commits into from
Nov 28, 2020

Conversation

yashha
Copy link
Contributor

@yashha yashha commented Nov 24, 2020

We found that the module.js does not support node version 10.x.
There seems to be an issue that TextDecoder is not imported and so cannot be found in that version.

The related ticket is:
danburzo/percollate#114

Best Regards

@mnater
Copy link
Owner

mnater commented Nov 24, 2020

Hi

First of all, thank you for the PR.
I understood what problem it solves, but I have my reservations about it.

  1. Node v10 reaches End of Life in a few months. Is it still worthwhile to support?
  2. If we decide to do so and if we import the TextDecoder from "util", the module is no longer compatible with Browserify and WebPack. So we would have to make a conditional require.

See also #153

What do you think?

@mnater mnater marked this pull request as draft November 24, 2020 22:30
@yashha
Copy link
Contributor Author

yashha commented Nov 24, 2020

Thanks for the quick response.

I would say when it is a quick fix, then we do the fix, because the EoL is not reached.
Why is TextDecoder from util is not compatible with Browserify and Webpack?
The tests run through properly, maybe we should add tests for browserify and webpack?

@danburzo
Copy link

If we decide to do so and if we import the TextDecoder from "util", the module is no longer compatible with Browserify and Webpack. So we would have to make a conditional require.

Yes, when I made the comment in the percollate PR, I was thinking along the lines of:

let TD = typeof TextDecoder !== 'undefined' ? TextDecoder : require('util').TextDecoder;

If indeed that's the only incompatibility with Node 10, it would be nice to support it.

As for the quesstion raised in #153 re: testing in Node LTS versions, maybe a simple GitHub Action like we have here might do the trick, at least for the tap-based tests?

Copy link
Owner

@mnater mnater left a comment

Choose a reason for hiding this comment

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

Nice! Thank you!

@mnater mnater marked this pull request as ready for review November 28, 2020 12:20
@mnater mnater merged commit fdb17bd into mnater:master Nov 28, 2020
@yashha yashha deleted the patch-1 branch November 28, 2020 16:26
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.

None yet

3 participants