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

Update README.md, fix test bug, update depends, replace broken Pow!, no curl to get libs #15

Merged
merged 6 commits into from
Mar 20, 2016

Conversation

grempe
Copy link
Contributor

@grempe grempe commented Jan 24, 2015

Fix a few issues I ran across when testing this locally. POW server is no longer supported for current versions of OS X, replaced with Caddy server. Updated all NPM dependencies. Fixed a bug where sniff-test for FileReader was not working for some node installations, scrypt-async is in NPM now, no need to pull in from head of master branch with curl, regenerated compiled files with latest version of coffescript.

Not all current NodeJS installs return "node" when checking
`process.argv[0]`.

On my system it returns:

$ node -p -e 'process.argv[0]'
/Users/glenn/.nvm/versions/node/v4.2.2/bin/node

Do the sniff-test of whether we are running tests in a browser or
in node a different way.

An example of previous fail was:

```
/Users/glenn/src/miniLockLib/library.compiled/readSliceOfData.js:17
        this.fileReader = new FileReader;
                              ^

ReferenceError: FileReader is not defined
    at EncryptOperation.module.exports [as readSliceOfData] (/Users/glenn/src/miniLockLib/library.compiled/readSliceOfData.js:17:31)
    at EncryptOperation.module.exports.EncryptOperation.encryptData (/Users/glenn/src/miniLockLib/library.compiled/EncryptOperation.js:161:19)
    at EncryptOperation.module.exports.EncryptOperation.run (/Users/glenn/src/miniLockLib/library.compiled/EncryptOperation.js:85:19)
    at EncryptOperation.module.exports.EncryptOperation.start (/Users/glenn/src/miniLockLib/library.compiled/EncryptOperation.js:78:16)
    at EncryptOperation.start (/Users/glenn/src/miniLockLib/library.compiled/EncryptOperation.js:4:59)
    at Object.miniLockLib.encrypt (/Users/glenn/src/miniLockLib/library.compiled/index.js:19:22)
    at /Users/glenn/src/miniLockLib/tests.compiled/A Few Demo Tests.js:15:26
    at /Users/glenn/src/miniLockLib/tests.compiled/fixtures.js:50:14
    at Object.read.files.basic.txt (/Users/glenn/src/miniLockLib/tests.compiled/fixtures.js:56:14)
    at exports.read (/Users/glenn/src/miniLockLib/tests.compiled/fixtures.js:46:28)
```
@grempe grempe changed the title Update README.md with more complete build and test instructions. Update README.md, fix test bug, update depends, replace broken Pow!, no curl to get libs Feb 5, 2016
@grempe
Copy link
Contributor Author

grempe commented Mar 2, 2016

Any chance of this being pulled upstream? I've heard nothing from @45678 on this pull request.

Thx!

@grempe
Copy link
Contributor Author

grempe commented Mar 17, 2016

ping? @45678

@45678
Copy link
Owner

45678 commented Mar 20, 2016

Thank you for working through these changes @grempe. And thank you for pursuing them with me for such an unreasonably long time. It is a very kind gesture. Thank you.

i haven’t been on the computer in a long time and that’s why i haven’t followed up with you. i am sorry that i didn’t notice this earlier because i know that you were left wondering – in limbo – for a long time.

i haven’t looked at the commits in detail yet but i will check them out later today or tomorrow morning. From the outside looking in they look great. Thanks again.

i will follow up with here you tomorrow.

Again, i am sorry about leaving you in limbo for so long. Kind regards.

@@ -1,4 +1,4 @@
if process?.argv?[0] is "node"
if typeof window is "undefined"
Copy link
Owner

Choose a reason for hiding this comment

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

Perfect, thank you for sorting this out @grempe.

@45678
Copy link
Owner

45678 commented Mar 20, 2016

Thank you for this thoughtful pull request @grempe. The presentation was great – it was very easy for me to read and follow the changes.

i made a few friendly amendments that are summarized in pull request #17.

@45678 45678 merged commit d1670d3 into 45678:master Mar 20, 2016
@grempe grempe deleted the patch-1 branch March 21, 2016 00:22
@grempe
Copy link
Contributor Author

grempe commented Mar 21, 2016

Thanks for pulling in the changes. Look forward to trying it out.

@45678
Copy link
Owner

45678 commented Mar 21, 2016

No problem @grempe. i was expecting more of your commits to show up in the master branch log. i don’t know how to use git and GitHub very well so i probably could have done the merge better to preserve more of your commits and due credit. anyway, sorry about that.

please do let me know if you run into any trouble with the master branch.

@grempe
Copy link
Contributor Author

grempe commented Mar 21, 2016

As far as I can tell all of my commits made it to master. I was able to cleanly delete my branch (which git would not let me do if there were unmerged changes).

Thanks. Hope these changes help the next guy. If there were a way to contact you out-of-band (twitter, email, whatever) that would have been helpful too :-)

The new web server changes work fine for me. Better than Pow.

Cheers,

Glenn

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