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

Make GoKibitz buildable again #186

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

apetresc
Copy link

@apetresc apetresc commented May 30, 2019

I'm attempting to build a development version of GoKibitz and finding that it's incompatible in a few ways with NodeJS 10.16.0 LTS. This PR attempts to make the smallest number of changes needed to allow GoKibitz to run cleanly with a modern Node toolchain.

This PR is not ready for merging, I'm just opening now to hopefully get some help overcoming a few hurdles.

  • Remove memwatch-next and heapdump dependencies
    memwatch-next is no longer supported and doesn't work under Node 10.16.0 (see this issue). Luckily it's not actually used for anything except a commented-out block of code. So removing the dependency doesn't break anything at all.
  • Upgrade gulp-sass to 4.0.2
    Again, the latest LTS NodeJS release isn't supported by node-sass<=4.9 as can be seen here.
  • Provide Docker Compose setup
    I wanna wrap the tricky native dependencies and running a persistent mongo instance behind Docker to make it easy to run on any system without messing with nvm and junk. The NodeJS toolchain is really atrocious...
  • Fix broken SCSS
    After getting a successful build, gulp default fails with:
    /Users/apetresc/src/personal/gokibitz/node_modules/node-sass/lib/index.js:291
       var payload = assign(new Error(), JSON.parse(err));
    Error: client/src/scss/rules/_layout.scss
    Error: Illegal nesting: Only properties may be nested beneath properties.
          on line 387 of client/src/scss/rules/_layout.scss
    >> 			$width: 3px;
    
       ---^
    
      at options.error (/Users/apetresc/src/personal/gokibitz/node_modules/node-sass/lib/index.js:291:26)
    
    The relevant section is:
        border: {
          $width: 3px;
          color: $flat-green-sea;
          radius: 0;
    //      radius: 0 0 0 $width * 2;
    //      width: $width;
          width: 0;
        }
    which looks like perfectly valid SCSS to me so I have no idea what's going on here. This is where I'm currently stuck.
  • Resolve the broken utf-8-validate module dependency
    At the moment, any npm build will contain a failure of the form:
    /Users/apetresc/.node-gyp/10.16.0/include/node/v8config.h:327:29: note: expanded from macro 'V8_DEPRECATED'
    declarator __attribute__((deprecated))
                                ^
    5 warnings and 1 error generated.
    make: *** [Release/obj.target/validation/src/validation.o] Error 1
    gyp ERR! build error
    gyp ERR! stack Error: `make` failed with exit code: 2
    gyp ERR! stack     at ChildProcess.onExit (/Users/apetresc/.nvm/versions/node/v10.16.0/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:262:23)
    gyp ERR! stack     at ChildProcess.emit (events.js:198:13)
    gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:248:12)
    gyp ERR! System Darwin 18.6.0
    gyp ERR! command "/Users/apetresc/.nvm/versions/node/v10.16.0/bin/node" "/Users/apetresc/.nvm/versions/node/v10.16.0/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
    gyp ERR! cwd /Users/apetresc/src/personal/gokibitz/node_modules/utf-8-validate
    gyp ERR! node -v v10.16.0
    gyp ERR! node-gyp -v v3.8.0
    gyp ERR! not ok
    
    Luckily this is an optional dependency so the build continues, but I'd still like to get to the bottom of it. It happens on both my local macOS and inside the Docker container so it's likely not something wrong with my build environment but rather another deprecation/incompatibility with Node 10.16.0.
  • Provide instructions in README about the correct format for .amazon-ses.js, or provide a way to mock/ignore them.
    At the moment, user.js fails to run without it.

I expect more problems to pop up after I get past the error above, but hopefully it can all be worked through 😃

memwatch-next is no longer supported and doesn't work under Node 10.16.0
(see [this issue](andywer/leakage#26)). Luckily
it's not actually used for anything except a commented-out block of code.
So removing the dependency doesn't break anything at all.
Again, the latest LTS NodeJS release isn't supported by node-sass <= 4.9
as can be [seen here](https://github.com/sass/node-sass#readme).
@neagle
Copy link
Owner

neagle commented May 31, 2019

I feel equal measures of gratitude and guilt for seeing someone else put part of their lives into a chore that I have put off myself. Thanks! I'd be happy to jump in and provide assistance if you'd be willing to give me permission to contribute to your branch while it's in PR status.

  • Docker: I've wanted to containerize the Mongo dependency, especially, for a while. This will be a big benefit for anyone else who wants to work on this locally. If you get a moment, or when it gets to that point, could you add instructions for running the Docker project?

  • The SCSS error: looks like Sass no longer tolerates a variable being declared in the middle of a nested properties block. The variable can just be moved one line above to fix the new error:

	.form-control {
		background: {
			color: $flat-green-sea;
		}
		$width: 3px;
		border: {
			color: $flat-green-sea;
			radius: 0;
                          // (...and so on)

@apetresc
Copy link
Author

My pleasure 😃 You should already have commit access to this branch specifically - let me know if it's not working for some reason and I can just make you a contributor on my entire fork.

  • Docker instructions have been added!
  • Thanks for the SCSS fix, that was indeed it.
  • I just edited the PR description with a few more TODOs; I'll just keep adding to the list as I encounter them.

@neagle
Copy link
Owner

neagle commented Jun 3, 2019

The README updates are fantastic; thank you!

I tried pushing to your branch and got an error--I think you have to manually allow me to make contributions, as described here. That's only if you'd like me to help you this way--if you'd rather collaborate by just having me make suggestions here, that's fine, too.

@apetresc
Copy link
Author

apetresc commented Jun 3, 2019

Yup, I had that checked since the beginning, that's why I thought it would work:

image

No big deal though, just added you as a collaborator on my fork so you can push to any branch including this one :)

@apetresc apetresc mentioned this pull request Nov 18, 2019
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.

2 participants