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

major linting overhaul #964

Closed
wants to merge 8 commits into from
Closed

major linting overhaul #964

wants to merge 8 commits into from

Conversation

daed
Copy link

@daed daed commented Jul 16, 2018

I spent some time today trying to fix the linter errors so that the travis build would show up successful again.

I fixed all of the ones I knew how to, and then downgraded the others to "warnings" in the eslintrc file.

Obviously this isn't great as far as review goes because I touched a ton of files in the process, but from what I can tell, I didn't break anything.

@daed
Copy link
Author

daed commented Jul 16, 2018

It looks like the node 6 test passed. Stable crashed though.

Maybe something related to this? nodejs/node#20285

@daed
Copy link
Author

daed commented Jul 18, 2018

The above changes work for the tests including node 8.11.3.

Fixing this to work on node 10.7.0 is going to be a huge undertaking. The core issue here is that there are packages (gulp 3.x, dependent on vinyl-fs which is dependent on glob-watcher which is dependent on gaze which is dependent on ... until you get to an ancient version of graceful-fs) that use unsupported/undocumented things from the natives package, and in node 10 they've restricted access to a lot of the api that was exposed though that package.

There's two workarounds I've found:

  • Force graceful-fs to use natives 1.1.4 via shrinkwrap. This fixes the issue in the short-term, but there's no guarantee it will continue to work in the future, because node seems to take an ill view on using natives.
  • Upgrade gulp to 4. This requires changing pretty much all of the gulps, and even after I tried it, it still didn't seem to work right. It's probably the "right" way to go, but it's also the longer route and would probably take someone who knows more about what they're doing than I do.

I don't know which way is the better way to go. For my own purposes, I'll probably just do the change for natives 1.1.4 and submit a PR for it anyway.

@daed
Copy link
Author

daed commented Aug 7, 2018

This needed some extra work. I managed to get the node 10 build to succeed, but I had to turn off some of the tests in the meanwhile. I'm not 100% sure whether it's the tests or the code itself that is failing, but functionality appears good in the program itself.

I need to do some more work before I'm comfortable with a PR on this.

@daed daed closed this Aug 7, 2018
@daed daed deleted the linting branch August 16, 2018 17:04
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.

1 participant