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

Random fixes required when I first checked out the project. #105

Merged
merged 8 commits into from Aug 14, 2012

Conversation

my8bird
Copy link
Contributor

@my8bird my8bird commented Jun 8, 2012

The only thing I see that could be problematic (through my naive eyes) would be the redis package as a dependency. In order to run the project redis is required so it seemed that this needed to be there.

Otherwise, this ensures that cake uses the projects coffee executable instead of the system wide one and adds some hooks to ensure the lib directory is up to date at crucial points.

@wmertens
Copy link
Contributor

wmertens commented Jun 8, 2012

Actually the redis dependency is optional... And if the system wide coffee matches the required version it won't be installed locally. It would be better to define a PATH that includes the node_modules here as well as the normal path and just call coffee. That way the shell will use the first one it finds.

@wmertens wmertens closed this Jun 8, 2012
@wmertens wmertens reopened this Jun 8, 2012
@wmertens
Copy link
Contributor

wmertens commented Jun 8, 2012

iPhone button booboo

@my8bird
Copy link
Contributor Author

my8bird commented Jun 8, 2012

I will torch the redis thing, it must just be the sharejs example that requires it. Also, I will look into the PATH extension idea (pretty sweet to know if that works as I can use it on some other projects).

@my8bird
Copy link
Contributor Author

my8bird commented Jun 8, 2012

@wmertens Updated to fix the issues you found.

@CrypticSwarm
Copy link
Contributor

Ideally just uses the prepublish, and not postinstall. Coffee should be moved as a dev dep only. It shouldn't be needed to run the server.

1 similar comment
@CrypticSwarm
Copy link
Contributor

Ideally just uses the prepublish, and not postinstall. Coffee should be moved as a dev dep only. It shouldn't be needed to run the server.

@josephg
Copy link
Owner

josephg commented Jul 16, 2012

@wmertens the lib directory already isn't part of git. The package's root in npm is index.js, which looks like this:

require('coffee-script');
module.exports = require('./src');

This means cake build isn't needed at all, and coffeescript is needed when you run the server.

The lib directory is only used for two purposes:

  • Building the webclient (via cake webclient). The webclient build concatenates a bunch of the client .js files together then uglifies them.
  • Debugging. When there's a crash, the line numbers are from the javascript file, not the .coffee file. So I often end up referring to the .js files in lib/ to figure out what line of coffeescript the crash is referring to.

@wmertens
Copy link
Contributor

wmertens commented Aug 1, 2012

So should the postinstall "cake build" be removed and the prepublish have "cake webclient"?

wmertens added a commit that referenced this pull request Aug 14, 2012
Random fixes required when I first checked out the project.
@wmertens wmertens merged commit 7adf7ec into josephg:master Aug 14, 2012
@wmertens
Copy link
Contributor

Thanks!

@josephg
Copy link
Owner

josephg commented Aug 14, 2012

Build is broken.

http://travis-ci.org/#!/josephg/ShareJS

@wmertens
Copy link
Contributor

Yup - it's because of sockjs, it broke as soon as I updated webclient a couple commits later. I have something that no longer breaks the tests but lets them last forever. I'm getting closer :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants