Skip to content

Update required node version to match the one installed #117

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

Merged
merged 1 commit into from
Aug 7, 2022
Merged

Update required node version to match the one installed #117

merged 1 commit into from
Aug 7, 2022

Conversation

internalsystemerror
Copy link
Member

Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA yes

Description

Dockerfile installed node v18, however package.json is using node v17. This PR updates the latter to ensure there is no confusion by developers or by the tools.

There are also a couple of removal instances of unused lines of code, which I can undo if requested. In this vein, I also added double quoting inside the shell scripts to prevent globbing and word splitting.

@internalsystemerror
Copy link
Member Author

internalsystemerror commented Aug 6, 2022

The target/lib version of ECMAscript to use was taken from: https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping. Node v18 will probably support es2022, however https://node.green/#ES2022 reports that this is only 97% implemented.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

🚢 build is also green

@Ocramius Ocramius self-assigned this Aug 7, 2022
@Ocramius Ocramius added this to the 1.13.0 milestone Aug 7, 2022
@Ocramius Ocramius merged commit 3570ac9 into laminas:1.13.x Aug 7, 2022
@internalsystemerror internalsystemerror deleted the upgrade-to-node-18 branch August 7, 2022 08:11
@Ocramius
Copy link
Member

Ocramius commented Aug 7, 2022

Well, post-merge build is red 😩

@Ocramius
Copy link
Member

Ocramius commented Aug 7, 2022

Nvm, seems like a docker pull limit issue, unrelated to this

@internalsystemerror
Copy link
Member Author

Odd that it was green in the PR.

I suspect you're correct, but https://github.com/laminas/laminas-ci-matrix-action/tree/1.14.x/tests/code-check-deprecated-exclusion-via-config is the failing test on both release branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants