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.

Sign up for GitHub

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

Jump to bottom

Simplify semantics and usage of copying and script inclusion #16

Closed
wants to merge 9 commits into from
Closed

Simplify semantics and usage of copying and script inclusion #16

wants to merge 9 commits into from

Conversation

iislucas
Copy link

Summary of changes:

  • Used code from grunt-contrib-copy for copying, so that cwd and expand are now supported, as well as other features like copy only if timestamps change (sadly couldn't use directly; grunt isn't very modular for code usage as far as I can see).
  • Separated the adding of script tags from the copying of files. This makes debugging path errors much easier, and makes them show up as console errors in the created chrome app instead of being mysteriously absent. (fixes: bad file paths simply result in "No Specs" but leave no errors on the console #14)
  • Files in the files parameter are now copied into a directory called files (instead of being copied into a directory called scripts)
  • Instead using the word paths in the options, and requiring it to be present, we now specify the scripts that get added to the HTML in a scripts parameter.
  • Adds an example and some more documentation to the README.md (fixes: Document files section of config for a target #15)
  • Updated package.json version to 2.0.0. (this is a usage-breaking change)

TESTED:

@soycode @willscott @bemasc @trevj

Review on Reviewable

@willscott
Copy link
Owner

Cool!

Here's the one design choice I was wrestling with where we came chose different options and maybe is worth discussion:
specifying an explicit scripts option to define what's linked in the HTML has the disadvantage that you can't easily use a glob rules because the files will have already been copied.
In a non-browserified project, the design pattern that would otherwise be natural would be to glob all of the *.spec.js files into the page.

willscott
willscott reviewed Mar 19, 2015
View reviewed changes
tasks/jasmine-chromeapp.js Outdated

function addFiles(files, to, tagFilter) {
var tags = '';
/* copyFiles is adapted from:
Copy link
Owner

Choose a reason for hiding this comment

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

This seems like it might make sense in a separate file. it's a rather involved method that could be used more generally.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure how to do that; there's dependencies on the grunt object within that code. Ideas?

Copy link
Owner

Choose a reason for hiding this comment

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

have a new file that does a module.exports = function(grunt) similar to how this one is structured?

or require('grunt') might work, since it looks like the dependence is mostly on static things like .util

Copy link
Author

Choose a reason for hiding this comment

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

module.exports = function(grunt) doesn't work (grunt is not defined except for top level required things, maybe grunt magic on its module registration), but require('grunt') does. Done.

@iislucas
Copy link
Author

Good idea w.r.t. support for *.spec.js. I'd suggest support for *.spec.js as a followup as it can be implemented separately... But when it gets added, I'd suggest having a check to give an error when glob resolves to the empty set of files (as its otherwise hard to debug what went wrong and what's missing: better to have explicit JS errors).

@iislucas iislucas closed this Dec 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@iislucas @willscott