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

Breaking changes for next major #2111

Closed
xzyfer opened this issue Oct 6, 2017 · 39 comments
Closed

Breaking changes for next major #2111

xzyfer opened this issue Oct 6, 2017 · 39 comments
Assignees
Milestone

Comments

@xzyfer
Copy link
Contributor

xzyfer commented Oct 6, 2017

Current WIP: #2312

We have been planning to release node-sass@v5 with LibSass 3.5.0 stable when it's ready. With a 5.0 milestone on the horizon we want to take note of the breaking changes we're planning to make.

This is living, and incomplete. Please subscribe for updates.

Node versions support

Moving forward we'll only be actively supporting active LTS and current Node versions. In practical terms this means Node 610+.

  • supporting old versions of Node and npm is a significant maintenance burden
  • older versions of npm are notoriously troublesome when it comes to native modules - it's becoming impractical, and sometimes even unsafe, to continue using old packages.

See #2290
See #2286
See #2257
See #2170
See #2256
See #2205
See #2288
...the list goes on

Switch watcher to node-chokidar

Why? Because Gaze in docker and various virtual machines uses a lot of resources whereas chokidar does not. Read about the advantages of chokidar

[...] in docker for mac you will get really high CPU usage with com.docker.hyperkit and com.docker.osxfs (I've seen reports of up to 300%).

See #2208

Compile on watch by default

When using the watch flag we should do a compilation before watching.
This becoming the expected behaviour in JS (see webpack).

See #2300
See #1973
See #1369
See #1742

Stop watching .css files

A LibSass has a feature/bug that allows it to @import .css files.
This means our watcher must also watch for .css files.
This can cause infinite loops if the input and out directories are the same.
LibSass is deprecating this behaviour.

See sass/libsass#2611
See #2184
See #2006
See #1933
See #1925
See #1867
See #1845

Don't fail installation on unsupported environments

In order to reduce issues about installation issues we failed the installation if we detected an environment we didn't provide binaries for.

This had the intended affect, but also had some unfortunate side-effects

  • difficult or impossible to install on environments that could fallback to local compilation i.e. arm, electron
  • required a version bump when new version of node landed
  • couldn't back port support for new Node versions

In v5 we should still produce an informative error, but allow the installation to continue.

@nschonni
Copy link
Contributor

nschonni commented Oct 6, 2017

For the node support version here is my proposal. Since Node publishes their LTS and dev branch timeline on https://github.com/nodejs/Release#lts_schedule

  • LTS minus 2 months, start spitting a warning on install warning that support is running out
  • LTS expired + 2 months, only allow install through a flag(--unsupporting-lts-bypass?), but warn no issues will be fixed
  • Dev/Odd number branches are no longer installable after new LTS branch date
  • Add curl/wget commands to the output for those that just can't/won't update

Not suggesting that binaries are removed after the LTS expiry, just that our install/download will error out and tell users to upgrade. This may change a little if/when WebAssembly is available, but I'm not going to assume that's a 5.x thing.

@nschonni
Copy link
Contributor

nschonni commented Oct 6, 2017

I think dropping/simplifying the binary options would also be a good idea. Just replace them with some curl/wget commands. Alternately, some instructions on symlinking for those that want the old site-layout style.

@nschonni
Copy link
Contributor

nschonni commented Nov 2, 2017

Maybe take a look at node-pre-gyp again as it looks like someone setup a GH releases piece https://github.com/bchr02/node-pre-gyp-github

@nschonni
Copy link
Contributor

nschonni commented Nov 2, 2017

Looks like node-pre-gyp doesn't have download caching right now, but there is a PR pending mapbox/node-pre-gyp#272

@bruce-one
Copy link

Just fyi, this repo supports node-pre-gyp for node-sass and publishes to github: https://github.com/bruce-one/node-sass (the binaries can be seen on the releases page). Eg npm i bruce-one/node-sass with node 8 installs using the binaries off github for me.

Just if of interest (never quite got round to making a PR :-s it's not very complicated though :-) also, fyi, the commit history is a mess because of trying to force travis builds. The diff isn't all that much.)

@nschonni
Copy link
Contributor

nschonni commented Nov 2, 2017

@bruce-one thanks! looks good, think we'd clean out a bunch of our custom SASS_BINARY stuff if we implement that. Also could rename the binding.node to the more standard node_sass.binding too. I won't do my own PR for now, but I'll ping you when we're ready for this 😄

@xzyfer
Copy link
Contributor Author

xzyfer commented Nov 2, 2017 via email

@xzyfer
Copy link
Contributor Author

xzyfer commented Nov 2, 2017

Platform support has been improved, but still doesn't match our supported platforms. Looks like there's also not support for electron or musl which is a bummer.

--target_platform=win32: Pass the target platform and override the host platform. Valid values are linux, darwin, win32, sunos, freebsd, openbsd, and aix.

Maybe we could look at PRing musl.

@nschonni
Copy link
Contributor

nschonni commented Nov 2, 2017

https://github.com/mapbox/node-pre-gyp#versioning

libc matches require('detect-libc').family like glibc or musl unless the user passes the --target_libc option to override.

@bruce-one
Copy link

I believe Electron support is handled via the runtime property rather than platform, and from what I know node-pre-gyp is used by Electron projects.

@xzyfer
Copy link
Contributor Author

xzyfer commented Nov 2, 2017 via email

@surfaceowl
Copy link

Even though tunnel-agent is only used for internal build per this closed issue, it would be helpful if request were upgraded >2.83.0 in node-sass@v5.

Package vulnerability scanners don't differentiate between packages used only for install vs. runtime... which means we have to mentally keep track of 'this one is okay' for tunnel-agent under node-sass / requests.

Would be great if this dependency upgrade could be included.

@nschonni
Copy link
Contributor

@surfaceowl I would like to remove request completely and use node-pre-gyp for 5, but I haven't looked to see if it is using request under the hood 😉

@realityking
Copy link
Contributor

@nschonni It is. Locked to 2.81.0.

@surfaceowl
Copy link

@nschonni -- that would help. Even though node-pre-gyp still uses request 2.81.0 it eliminates the current package vulnerability.

@xzyfer
Copy link
Contributor Author

xzyfer commented Mar 24, 2018

Update the issue description with additional changes in v5.

@xzyfer xzyfer mentioned this issue Apr 3, 2018
14 tasks
@xzyfer
Copy link
Contributor Author

xzyfer commented Apr 4, 2018

Work on this has begin in #2312

@nschonni
Copy link
Contributor

nschonni commented Apr 4, 2018

@bruce-one we've created the v5 branch now if you want to PR your node-pre-gyp stuff now ❤️

@xzyfer
Copy link
Contributor Author

xzyfer commented Apr 5, 2018

I'm growing concerned that the Active LTS and Current support target may be

  • complicated to communicate, and
  • too aggressive

Looking at our release binary download stats, Node 4 (46) is getting a significant number of downloads.

We're forced to drop Node < 4 due to our dependencies. And we agree that using Node < 4 is unsafe and should be discouraged.

There are some Node milestones coming up in April

  • Node 4 reaches EOL on the 30th, and
  • Node 10 is due to be stable on the 24th

Given that we want to get a final 4.x release out with a LibSass bump. We should take this opportunity to add the Node 10 binaries, and monitor the release binary download numbers.

With any luck there will be a significant drop off in Node 4 installs. If not I would be more comfortable aiming for Node >= 4 support.

Reference

You can see the download stats with the following query in GitHub's GraphQL API explorer.

query { 
  repository(owner: "sass", name: "node-sass") {
    releases(last: 5) {
      nodes {
        name,
        releaseAssets(first: 100) {
          nodes {
            name,
            downloadCount
          }
        }
      }
    }
  }
}

@realityking
Copy link
Contributor

realityking commented Apr 5, 2018

I crunched the numbers for one release (4.7.2 to be exact) really quick. This might warrant digging a bit more (e.g. more release), so here's the code: https://gist.github.com/realityking/5f29d001ed96f1e9e6a318bb71122508

The result I got is this:

Release Download Count Percentage
Node 0.10.x 83,180 0.35%
Node 0.12.x 68,291 0.29%
io.js 1.x 51,943 0.22%
io.js 1.1.x 52,395 0.22%
io.js 2.x 52,064 0.22%
io.js 3.x 12,960 0.05%
Node.js 4.x 464,501 1.96%
Node.js 5.x 246,400 1.04%
Node.js 6.x 7,469,360 31.57%
Node.js 7.x 1,614,719 6.83%
Node.js 8.x 10,794,921 45.63%
Node.js 9.x 2,746,781 11.61%
Total 23,657,515 100%

I'll leave the decision-making to others 😄

@nschonni nschonni added this to the 5.0 milestone Dec 20, 2018
@nschonni nschonni removed the v5 label Dec 20, 2018
@deavial
Copy link

deavial commented Jan 16, 2019

are there any plans to drop node-gyp for cmake-js (or other) eliminating the requirement of python 2.7 to be installed?

@xzyfer
Copy link
Contributor Author

xzyfer commented Jan 16, 2019 via email

@gtmills
Copy link

gtmills commented May 11, 2019

Is there a release date for version 5?

@saper
Copy link
Member

saper commented Oct 21, 2019

the description ^^ and the TROUBLESHOOTING.md file still say

As of node-sass@v5 only Node 6 and above will be officially supported.

As of 21 October 2019, Node 8 is the oldest LTS. Does this mean we drop node 6? That would mean finally dropping CentOS 5 \o/ (despite what the release page says, node 7 does not work on CentOS5).

Do we drop node 6?

@xzyfer
Copy link
Contributor Author

xzyfer commented Oct 21, 2019 via email

@deavial
Copy link

deavial commented Oct 21, 2019

Node 10 isn’t universally available on all cloud platforms yet. Even in google it is only available as an unsupported beta. Only certain services on AWS have support. Removing Node 8 while it is in LTS could cause an uproar.

@saper
Copy link
Member

saper commented Oct 21, 2019

I have went through a lot of issues on our tracker during last few days and I have my doubts about watcher changes. I suspect our watcher support in node-sass CLI is pretty broken and replacing the watcher engine will not help. I am not sure I can attribute any of the open issues to gaze, most of them are related to the ways we expand the list of the watched files.

I'll try to do something about it but I suspect it is too much work to fit into the nearest breaking release.

@nschonni
Copy link
Contributor

Removing Node 8 while it is in LTS could cause an uproar.

The EOL for 8 is in December https://github.com/nodejs/release

@deavial
Copy link

deavial commented Oct 21, 2019

The EOL for 8 is in December

Eh, didn't know that. I retract my objection then.

@saper
Copy link
Member

saper commented Oct 24, 2019

I think dropping/simplifying the binary options would also be a good idea. Just replace them with some curl/wget commands.

Yes, it's a mess we should drop ASAP. What does not help is that our release binding file name is not what we import, maybe we should change this somehow?

@saper
Copy link
Member

saper commented Oct 24, 2019

Stats for version 4.12.0 (available since April 2018, using @realityking's gist, updated)

Release Download Count Percentage
Node 0.10.x 145,643 0.20%
Node 0.12.x 135,675 0.19%
io.js 1.x 128,246 0.18%
io.js 1.1.x 128,837 0.18%
io.js 2.x 64,486 0.09%
io.js 3.x 129,188 0.18%
Node.js 4.x 189,784 0.26%
Node.js 5.x 174,506 0.24%
Node.js 6.x 2,054,788 2.86%
Node.js 7.x 383,967 0.54%
Node.js 8.x 17,821,901 24.84%
Node.js 9.x 1,098,451 1.53%
Node.js 10.x 37,469,429 52.23%
Node.js 11.x 4,953,869 6.91%
Node.js 12.x 6,857,638 9.56%
Total 71,736,408 100%

@nschonni
Copy link
Contributor

I think dropping/simplifying the binary options would also be a good idea. Just replace them with some curl/wget commands.

I want to use node-pre-gyp, but losing the caching we do here is a problem. I don't know if there is a consistent story for caching anywhere for the pre-built addi-ins (still)

@GeorgeTaveras1231
Copy link

In v5, will anything change for the importer option that has been documented as experimental for a while?

@saper
Copy link
Member

saper commented Mar 11, 2020

In v5, will anything change for the importer option that has been documented as experimental for a while?

No changes in the API here.

@PolyPik
Copy link

PolyPik commented Mar 16, 2020

I have updated the libsass version on branch v5 to 3.6.3. #2859

@surfaceowl
Copy link

@nschonni --- node-pre-gyp no longer uses request, it was replaced by needle in this PR. Here is current package.json for node-pre-gyp

@lod3456
Copy link

lod3456 commented Apr 22, 2020

when will V5 be released?

@Abdullah0820

This comment has been minimized.

@nschonni nschonni changed the title Breaking changes for version 5 Breaking changes for next major Nov 23, 2020
@nschonni nschonni unpinned this issue Apr 23, 2021
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

No branches or pull requests