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

Friendly amendments to pull #15 #17

Merged
merged 13 commits into from
Mar 20, 2016
Merged

Friendly amendments to pull #15 #17

merged 13 commits into from
Mar 20, 2016

Conversation

45678
Copy link
Owner

@45678 45678 commented Mar 20, 2016

Hi @grempe, here are some amendments to the changes you pushed in pull request #15.

If these are ok with you i will merge them into the master branch. Or feel free to make some suggestions for additional changes.

Thank you again for your work on this. Kind regards.

grempe and others added 11 commits January 24, 2015 12:04
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)
```
…ADME.

Revised the introduction of the Digging In section to explain that GNU
Make and Node.js are the basic software requirements for fiddling with
the miniLockLib code.

Removed pointer to “brew install …” because i don’t like alcohol or the
command line.
Running `make` works on my computer when the browserify package is
installed locally with `npm`. i am willing to consider other options if
this doesn’t work predictably on other computers.
@grempe
Copy link
Contributor

grempe commented Mar 20, 2016

Hi, glad you are still there. I guess its intentional that you are trying to completely obfuscate your identity and association with this lib? ;-)

Regarding Pow. It would not work for me with a clean install on OS X El Capitan. There are a lot of comments on several Pow issues related to this:

basecamp/pow#452
basecamp/pow#517
basecamp/pow#521

The last commit to Pow repo was in 2014. It is fair to consider it abandoned and non-functional due to significant changes by Apple in the OS X releases since 2014.

https://github.com/basecamp/pow/commits/master

Caddy is a super simple replacement web server. It really doesn't matter what you use I suppose, maybe even a simple python command to run a local server. But please don't leave to Pow instructions in place.

e.g.
http://www.linuxjournal.com/content/tech-tip-really-simple-http-server-python

According to the browserify site it should be installed as a global since it provides a system wide binary:
http://browserify.org/#install
http://stackoverflow.com/questions/35992104/why-is-it-necessary-to-install-browserify-twice-to-bundle

docco should also be installed global according to their docs (again with the rule of binary's being installed globally, and packages you will require being installed locally):
http://jashkenas.github.io/docco/

Thanks again for responding and looking carefully.

Glenn

@45678
Copy link
Owner Author

45678 commented Mar 20, 2016

Thanks for pointing out the abandon-ed-ness of Pow. i didn’t know about any of that stuff. i will phase it out of this project to guard against anymore confusion.

Rather than add install instructions and config for Caddy, i decided to add the 'http-server' package to the development dependencies in the master branch (ede5997) so that everything required to run the tests is available after npm install (hopefully at least).

To start up the default webserver to facilitate the window tests type npm run webserver. You still use Caddy if you prefer it, but you may need to re-define the window tests address. The default address is http://localhost:45678/tests.html. It can be re-defined with the .window_tests_address file.

@45678
Copy link
Owner Author

45678 commented Mar 20, 2016

thanks for bringing up the issue about installing packages globally @grempe.

i see that docco and browserify recommend a global install but i don’t want to recommend it for the miniLockLib setup procedure. they both work good for me as local development dependencies and i would prefer to keep the setup procedure uncomplicated. i just want to git clone and npm install and then get to work.

of course, if that flow is problematic for you i'm willing to reconsider. for now i'm going to leave it unmentioned in the README i think.

@45678 45678 merged commit 4896142 into master Mar 20, 2016
@grempe
Copy link
Contributor

grempe commented Mar 21, 2016

Sounds good. I was not particularly tied to Caddy. Just suggesting a working replacement. Your solution sounds fine. I'll try it out.

Also fine for docco and browserify, as long as they work just installed locally. :-)

G

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.

None yet

2 participants