Change non-git shell script files to node files #320

Open
wants to merge 5 commits into
from

Projects

None yet

3 participants

@mattferrin

I think this is a good step in the right direction. I tested it on my Windows an Mac machines.

The most controversial change, I switched the example to run on port 8081 so that it is different from the port used in the main non-example.

@facebook-github-bot

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@facebook-github-bot

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@wincent wincent added a commit to wincent/graphiql that referenced this pull request Feb 9, 2017
@wincent wincent Use random port numbers in test and example apps
graphql#320 suggested changing the
hard-coded port number in the example server so that it doesn't collide
with the test app.

Instead of that, let's use random port numbers, which will strictly
avoid any port collisions, including with apps that have nothing to do
with this repo, because the OS will assign an unused port.

Tested by simultaneously launching:

    # In one terminal:
    cd example
    npm install
    npm run start # prints: Started on http://localhost:51638/

    # In another terminal:
    npm run dev # prints: Started on http://localhost:51578/

Then hit both links and see they work.
c046bea
example/server.js
@@ -15,4 +15,4 @@ const app = express();
app.use(express.static(__dirname));
app.use('/graphql', graphqlHTTP(() => ({ schema })));
-app.listen(8080, () => console.log('Started on http://localhost:8080/'));
+app.listen(8081, () => console.log('Started on http://localhost:8081/'));
@wincent
wincent Feb 9, 2017 Contributor

As this is unrelated, should probably be pulled out into a separate commit.

In any case, I actually think we should use a port number of 0 here, which will cause the OS to assign a random port number. I've just sent a PR for that: #321

@wincent
Contributor
wincent commented Feb 9, 2017

Thanks for taking the time to get things working on Windows, @mattferrin! I might add a comment or two in line, but here are some high level remarks to start with:

This is a good start. I think, however, that we should address my suggestions from #318 before merging this. It seems quite wrong, for example, to be shelling out to the echo executable from node when we could instead just print to standard out with process.stdout.write. Likewise, there are "native" Node equivalents for much of the shell stuff we are doing in here (the fs.* methods I suggested in the other issue) and existing, mature abstractions to replace forking out to platform-specific commands (for example, we could use rimraf instead of rm -rf/rd).

This isn't so much a "port to node" so much as a "writing bash in node", which is pretty painful to write, and even read (due to all the escaping). I'm happy to help out with doing an a node-centric rewrite if you're not up for it.

Thanks once again for your time on this!

resources/prepublish.js
+ // # Publishing to NPM is currently supported by Travis CI, which ensures that all
+ // # tests pass first and the deployed module contains the correct file structure.
+ // # In order to prevent inadvertently circumventing this, we ensure that a CI
+ // # environment exists before continuing.
@wincent
wincent Feb 9, 2017 Contributor

Can you remove this Bash comment syntax from inside your JavaScript comments?

@mattferrin
mattferrin Feb 9, 2017

Gotcha. Done :)

@wincent wincent added a commit that referenced this pull request Feb 9, 2017
@wincent wincent Use random port numbers in test and example apps (#321)
#320 suggested changing the
hard-coded port number in the example server so that it doesn't collide
with the test app.

Instead of that, let's use random port numbers, which will strictly
avoid any port collisions, including with apps that have nothing to do
with this repo, because the OS will assign an unused port.

Tested by simultaneously launching:

    # In one terminal:
    cd example
    npm install
    npm run start # prints: Started on http://localhost:51638/

    # In another terminal:
    npm run dev # prints: Started on http://localhost:51578/

Then hit both links and see they work.
0199a44
@wincent wincent self-assigned this Feb 9, 2017
@mattferrin

I've removed some bash style comments because I could swear I was asked to, though I can no longer seem to find that request to comment that it's done.

As for the port to Node file system functions, I do not have the time for it myself. My goal is to keep my code as close as possible to the existing and working solution and not break things.

@wincent
Contributor
wincent commented Feb 9, 2017

I've removed some bash style comments because I could swear I was asked to, though I can no longer seem to find that request to comment that it's done.

Hidden away here.

Thanks for the updates!

@mattferrin

FYI, I'm doing a little more work on this even though I said I didn't have time, and I don't! :)

@mattferrin

@wincent Your suggestion

Well, it is literally just concatenating the files, so could get the listing with fs.readdirSync, read the files with fs.readFileSync, concat in JS, then fs.writeFileSync the result.

wasn't implemented because I don't know how to do it while file globbing.

@mattferrin

@wincent Apologies. I'm blind. You explicitly state I can use readdirSync. I'll do that. Thanks.

@mattferrin

I think I've completed all the suggestions. If you would like I can soft reset all my commits and make it one commit, after we're done collaborating on this of course.

@wincent
Contributor
wincent commented Feb 11, 2017

FYI, I'm doing a little more work on this even though I said I didn't have time, and I don't! :)

I know the feeling well.

I think I've completed all the suggestions. If you would like I can soft reset all my commits and make it one commit, after we're done collaborating on this of course.

That won't be necessary. GitHub has a shiny "Squash and merge" button. I have reviewed the new changes yet, but will do so shortly. Thanks for taking this extra stab at it!

+console.log('"Running node ./resources/build.js"');
+
+if (!isWindows) {
+ out('set -e'); // exit on console error
@wincent
wincent Feb 11, 2017 Contributor

I don't think this will do anything, will it?

out will fork a child process that lives for the duration of the set command, then exit, so the attempt to change the setting will have no subsequent effect.

I think we can delete this.

@mattferrin
mattferrin Feb 14, 2017

I remember it halting program execution upon an error, but I need to find time to test and prove my memory is either correct or incorrect.

@wincent
wincent Feb 14, 2017 Contributor

Sounds good. I would be extremely surprised if it has any effect.

+fs.mkdirSync(distDir);
+
+out('babel src --ignore __tests__ --out-dir dist/');
+console.log('"Bundling graphiql.js..."');
@wincent
wincent Feb 11, 2017 Contributor

Why the double quotes here?

@mattferrin
mattferrin Feb 14, 2017

They are accidental remnants from past echo commands. I can remove them, but noticed they seemed to help contrast between other console writes and made these easier to read even if the quotes lacked meaning. (There are JS filenames logged to console during the build without quotes.)

+out('babel src --ignore __tests__ --out-dir dist/');
+console.log('"Bundling graphiql.js..."');
+out('browserify -g browserify-shim -s GraphiQL dist/index.js > graphiql.js');
+console.log('"Bundling graphiql.min.js..."');
@wincent
wincent Feb 11, 2017 Contributor

And here (etc; I won't bother commenting on the other places).

@mattferrin
mattferrin Feb 14, 2017

Confirm you'd like me to remove them and the double quotes will be gone :)

@wincent
wincent Feb 14, 2017 Contributor

Yeah, I think you should.

+if (isWindows) {
+ out('browserify -g browserify-shim -g uglifyify -s GraphiQL dist/index.js | uglifyjs -c --screw-ie8 > graphiql.min.js');
+} else {
+ out('browserify -g browserify-shim -g uglifyify -s GraphiQL dist/index.js 2> /dev/null | uglifyjs -c --screw-ie8 > graphiql.min.js 2> /dev/null');
@wincent
wincent Feb 11, 2017 Contributor

I think we can do this without piping to the shell but using real pipes in a platform-agnostic way, so we won't need the isWindows check here.

@mattferrin
mattferrin Feb 14, 2017

You're right. It might take a while for me to get around to doing this.

@@ -0,0 +1,45 @@
+var execSync = require('child_process').execSync;
@wincent
wincent Feb 11, 2017 Contributor

Just a thought: you could use ES6 features in this file if you executed it with babel-node instead of node.

@mattferrin
mattferrin Feb 14, 2017

It's a good idea. I'll definitely do that on personal projects moving forward, but am unsure it's best here.

@mattferrin
mattferrin Feb 14, 2017

Say to do it and I'm game though.

@wincent
wincent Feb 14, 2017 Contributor

We have Babel in the project precisely so that we can use ES6 features, so doing so would be consistent with the other usage in the repo.

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