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

feat: Add Node 16 CI and drop Node 10 #3090

Merged
merged 4 commits into from
May 8, 2021
Merged

feat: Add Node 16 CI and drop Node 10 #3090

merged 4 commits into from
May 8, 2021

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Apr 20, 2021

  • Bump to 5.1.0
  • Add CI builds for 16
  • Add extension lookup for module 93

Fixes #3077
Fixes #3094

@nschonni
Copy link
Contributor Author

Node 16 alpine image isn't out yet, so I expect this to fail for now

@pedrofurtado

This comment has been minimized.

@nschonni
Copy link
Contributor Author

@xzyfer looks like there is an internal compiler issue with some of the headers on macOS and Linux. Does this look like a Nan thing, or maybe a compiler version one. I tried bumping to GCC 8 since its' the new minimum, but I don't think it helped

@saper
Copy link
Member

saper commented Apr 21, 2021

  • node 16 alpine containers do not seem to be published there yet
  • FreeBSD did not merge in node 16 into the ports system commit

@saper
Copy link
Member

saper commented Apr 21, 2021

Experimenting with #3091

@xzyfer
Copy link
Contributor

xzyfer commented Apr 21, 2021

Looks like the OSX failure might have a fix https://github.com/microsoft/node-pty/pull/433/files

@saper
Copy link
Member

saper commented Apr 21, 2021

#3091 seems to fix this, I wonder if we should be using the same compiler settings for all supported versions...

@nschonni nschonni changed the title node-16 feat: Add Node 16 CI Apr 24, 2021
package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "node-sass",
"version": "5.0.0",
"version": "5.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saper @xzyfer should we just bump this to 6 and also drop Node 10 with #3094? Would mean maybe waiting till the end of the week for cutting the release

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to have more research about actual compilers and C++ standards we need. Hope we do not need to use different once for different node versions?

I think one node version out, one node version in is a fair trade, we could do 6.x this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dropped the conditionals entirely here, and the CI seems OK, but that's no guarantee 😄

Copy link
Member

Choose a reason for hiding this comment

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

We should with the same settings as node does. Maybe they are inherited from the node gyp files, we should check this. Long time since I've had look into this...

The c++0x workaround was for the old gcc on MacOS X I think...

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with bumping to 6 :)

- Bump to 5.1.0
- Add CI builds for 16
- Add extension lookup for module 93
- VS2015 isn't supported anymore
- CPP11 flags cause issues on modern tooling
@nschonni
Copy link
Contributor Author

nschonni commented May 1, 2021

@xzyfer @saper I added the dropping on Node 10, and bumped it to 6.0.
Do you want to still work on the compiler stuff, or try the trimmed down one and maybe add back if we get reports coming in?

@nschonni nschonni changed the title feat: Add Node 16 CI feat: Add Node 16 CI and drop Node 10 May 1, 2021
@xzyfer
Copy link
Contributor

xzyfer commented May 2, 2021 via email

@xzyfer
Copy link
Contributor

xzyfer commented May 2, 2021 via email

@nschonni
Copy link
Contributor Author

nschonni commented May 2, 2021

Alpine 16 seems to have some flakey CLI tests, but I think we say that in 15 for awhile too

@nschonni
Copy link
Contributor Author

nschonni commented May 2, 2021

Feel free to land this whenever you have time to cut the release. I bumped the package.json this time 😄

@xzyfer
Copy link
Contributor

xzyfer commented May 2, 2021 via email

@xzyfer xzyfer merged commit 886319b into sass:master May 8, 2021
@nschonni nschonni deleted the node-16 branch May 8, 2021 14:59
@xzyfer
Copy link
Contributor

xzyfer commented May 8, 2021

Released v6.0.0 with Node 16 support.

@stevecondylios
Copy link

stevecondylios commented May 9, 2021

Hi there. Thanks for the contributors' support on a tough issue under pressure.

sass/node noobie here. Just wondering, is this problem history now? i.e. is it okay to update to node 16?

From what I read above, I assume it might be okay to go to node 16 now, (although I do see 1 failed check for build 16 (I'm not sure if that affects things): https://github.com/sass/node-sass/runs/2483549702)

Some days back I wrote this quick suggestion for anyone not sure what to do, but if it's now all good to go to node 16, I will update that answer to avoid node 14 and go to 16 now :) Please let mek know if my understanding is correct?

@gotjoshua
Copy link

@stevecondylios

okay to update to node 16

not on mac osx its not.

@asharafshahi
Copy link

How about now? If I install the latest will it work with node 16?

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.

Drop Node 10 when it hits EOL on 2021-04-30 Node 16 support
7 participants