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 Node 10 support #1493

Merged
merged 1 commit into from
Dec 14, 2020
Merged

Fix Node 10 support #1493

merged 1 commit into from
Dec 14, 2020

Conversation

gaetanmaisse
Copy link
Contributor

What kind of change does this PR introduce?

Bugfix: replace use of Array.prototype.flat() to ensure Node 10 compatibility

Did you add tests for your changes?

No

Summary

As stated in the Readme and in the engine property of the package.json, preact-cli 3.0.x should be compatible with Node 10. However, https://github.com/preactjs/preact-cli/pull/1467/files#diff-490a9490c1fcd253d926ffeaea6b185b15522512e53f6a9f36493122fcae9d2eR4 introduced the use of Array.prototype.flat() (in validate-args.js file) which is available on Node 11+.

To work around that and ensure Node 10 compatibility I replaced it with a good old reduce + concat.


Here is the current issue when running npx preact-cli create preactjs-templates/default preact-latest with Node 10:

✖ ERROR TypeError: options.map(...).flat is not a function
    at validateArgs (/Users/gmaisse/.npm/_npx/13643/lib/node_modules/preact-cli/lib/commands/validate-args.js:4:72)
    at command (/Users/gmaisse/.npm/_npx/13643/lib/node_modules/preact-cli/lib/commands/create.js:210:2)
    at Sade.parse (/Users/gmaisse/.npm/_npx/13643/lib/node_modules/preact-cli/node_modules/sade/lib/index.js:189:56)
    at Object.<anonymous> (/Users/gmaisse/.npm/_npx/13643/lib/node_modules/preact-cli/lib/index.js:83:6)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:831:12)

Does this PR introduce a breaking change?

No, it fixes one.

@changeset-bot
Copy link

changeset-bot bot commented Dec 9, 2020

⚠️ No Changeset found

Latest commit: 6400078

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@gaetanmaisse gaetanmaisse marked this pull request as ready for review December 9, 2020 21:26
@gaetanmaisse gaetanmaisse requested a review from a team as a code owner December 9, 2020 21:26
@yannbf
Copy link

yannbf commented Dec 11, 2020

Hey @prateekbh do you think you could maybe check this out? Thanks!

@rschristian
Copy link
Member

rschristian commented Dec 11, 2020

@gaetanmaisse Nice catch.

Do you mind adding 10.x to the CI version matrix? It would help prevent regressions.

Actually #1050 has me wondering if Node 10 is really something meant to be supported still.

@gaetanmaisse
Copy link
Contributor Author

Node 10's end of life is 2021-04-30 so it feels right to not spend a lot of time to ensure compatibility.
My opinion would be to merge this fix to ensure 3.x is still compatible with Node 10 and in the next few months stop the support of Node 10, update readmes, engine in package.json and publish a 4.x. But I really don't know what is the process in the preact ecosystem.

What's your opinion on that?

Note: I discovered the problem with Storybook E2E tests as we are bootstrapping a fresh preact project and all our E2Es are currently running on Node 10 because the current version of Storybook is targeting Node 10+.

@prateekbh
Copy link
Member

I mean this PR itself is not a huge deal, but I do remember that it was a conscious call to keep the support to 12.0.

I would not want to add 10.x to the matrix as that would mean that everyone needs to comply with that.

@gaetanmaisse
Copy link
Contributor Author

Not sure to understand what I need to do then, revert the matrix change but keep my fix? or closed this PR, update the readme + package.json and ask for a major release?

@rschristian
Copy link
Member

Revert the matrix, yeah. I wasn't privy to the conversation about support to 12.0 and I only caught how close we were to it being EOL after I sent the message originally.

Engine stuff will certainly need to be updated eventually.

…ility

As stated in the Readme and in the `engine` property of the `package.json`, `preact-cli` 3.0.x should be compatible with Node 10.
However, ec760ab introduced the use of `Array.prototype.flat()` (in `validate-args.js` file) which is available on Node 11+.
To work around that and ensure Node 10 compatibility I replaced it with a good old `reduce` + `concat`.
@gaetanmaisse
Copy link
Contributor Author

Ok, I just forced push to clean the branch and only get the fix commit ;)

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.

4 participants