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

Fix node10 compatibility #46

Merged
merged 2 commits into from
Jun 19, 2021

Conversation

adriencaccia
Copy link
Contributor

Use lodash flatten instead of Array.prototype.flat to fix the issue.

To prevent future issues, I lowered ts lib to match runtime requirements.

Fix: #38

@t-richard
Copy link
Contributor

t-richard commented Jun 18, 2021

I'm not sure node 10 should be supported.

Node 10 is EOL since a couple of months and AWS lambda support is ending in August 2021. See this AWS docs page.

Why do you think this is needed ?

EDIT : I know AWS Lambda support has nothing to do here because Lift would not be executed on lambda as it's used only for infrastructure but I don't think people would be using Node10 in their Serverless projects if Lambda support is over.

@mnapoli mnapoli added the bug Something isn't working label Jun 18, 2021
@mnapoli
Copy link
Member

mnapoli commented Jun 18, 2021

Thanks a lot @adriencaccia for starting this!

I'm a bit torn too. I'll try to see internally if we have any data about Node 10 usage, to see if we should support it.

The thing is that this PR doesn't fix all the problems (AFAICT): we use flat() in several places, and it's not impossible there are other places too where we use code not compatible with Node 10.

@adriencaccia
Copy link
Contributor Author

adriencaccia commented Jun 19, 2021

The thing is that this PR doesn't fix all the problems (AFAICT): we use flat() in several places,

This is my bad, I forgot to lint and type check my code before submitting the PR. I force-pushed changes to replace all usage of flat().

it's not impossible there are other places too where we use code not compatible with Node 10.

Thanks to my second commit, both eslint and tsc will now throw errors when using code not compatible with node 10. That is why the CI failed when I first submitted the PR.

Why do you think this is needed ?

This PR intent to make it possible for users to use the plugin with node 10. Even if there are not that many, they will have a better experience.
As you can see, the cost to reach compatibility is not that high, here we only have to stop using flat() and use lodash's flatten.
And by lowering TS lib to es2017, we assure that code that would break compatibility will not be introduced.

@mnapoli
Copy link
Member

mnapoli commented Jun 19, 2021

Hey @adriencaccia, the lib: "es2017" seems to be a very good solution indeed! Just tried it locally and it showed the errors as you mentioned. I learned something cool today 🙏

I really like your PR: we can support Node 10 with very little effort, it seems like a no-brainer to merge. And the CI validation is perfect.

Thanks a lot!

@mnapoli mnapoli merged commit 2d53746 into getlift:master Jun 19, 2021
@adriencaccia adriencaccia deleted the enable-node10-compatibility branch June 19, 2021 11:23
@t-richard t-richard mentioned this pull request Oct 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node 10 compatibility ("flat is not a function")
3 participants