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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix callback arguments on compile, so we trigger dependencies correctly #1

Merged
merged 1 commit into from Aug 26, 2015

Conversation

lalomartins
Copy link
Contributor

After about an hour debugging why brunch wasn't picking up changes on my include files 馃槈 I realised you're calling the callback with invalid arguments. By returning (null, '') we trigger getDependencies correctly.

@stephenhutchings
Copy link
Owner

Thanks @lalomartins I'll look at this when I have a moment spare.

@stephenhutchings
Copy link
Owner

@lalomartins Can you tell me more about your environment? Brunch version, etc. These changes seem to make no difference to me. My projects run correctly, with changes to included jade files triggering compilation.

@lalomartins
Copy link
Contributor Author

I'm using it in the office, and I'm not there today, and I don't have access to the repos from outside :-( so I can let you know for sure Monday. But I can tell you from memory: Ubuntu 14.04, Node 0.12.7, latest brunch installed via npm, latest jade-static-brunch via npm (both local installs).

@lalomartins
Copy link
Contributor Author

There's a discussion about it at brunch/brunch#916 and, well, looking at the brunch source, I don't see how it can possibly be working for you, unless you're using a version of brunch < 1.7.0.

See also brunch/brunch#557

@stephenhutchings
Copy link
Owner

stephenhutchings commented Aug 14, 2015 via email

lalomartins added a commit to lalomartins/jaded-brunch that referenced this pull request Aug 18, 2015
@lalomartins
Copy link
Contributor Author

Since you created this repo this year according to the commit history, it's definitely targeting brunch > 1.7.0; and yet, looking at the brunch sources, I can't imagine how it could possibly work 馃榾 what a mystery.

Anyway, there seems to be some support for adding static file generation to brunch core, and I'll probably be starting a branch soon (already forked the repo even). So if you're not sure about merging, it's probably safe to just leave it alone for now (and those who need it can just use it from my branch via git reference in package.json, which is what I'm doing here).

There is in fact the unfortunate side-effect that it generates an empty JS module in the output (which I worked around by joining it to a dummy.js file). So maybe we need to think a bit more about this.

@stephenhutchings
Copy link
Owner

Ok, cheers. I'm going to leave it open for now and will try to get to the bottom of it at some point when I have more time.

@ceremcem
Copy link

this pull request seems to fix both #3 and #4

This was referenced Aug 24, 2015
@stephenhutchings
Copy link
Owner

Took a quick look at this today and these changes no longer pass the included tests, which do cater for includes. Are you getting the same results?

@ceremcem
Copy link

I didn't get it. You got what result?

@stephenhutchings
Copy link
Owner

Scratch that last comment. I must of been doing something stupid to get those errors. This is seems fine to me and am happy to merge, even regarding:

the unfortunate side-effect that it generates an empty JS module in the output

stephenhutchings added a commit that referenced this pull request Aug 26, 2015
fix callback arguments on compile, so we trigger dependencies correctly
@stephenhutchings stephenhutchings merged commit 9735427 into stephenhutchings:master Aug 26, 2015
@ceremcem
Copy link

Yes, it generates an empty JS module with the content of last-unmatched-jade-file, but it doesn't hurt. I'm simply ignoring that file for now.

@stephenhutchings
Copy link
Owner

FYI, this is now available in v1.8.3 on NPM.

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

3 participants