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

"explicit parent script" not working #20

Closed
boneskull opened this issue Apr 16, 2016 · 14 comments
Closed

"explicit parent script" not working #20

boneskull opened this issue Apr 16, 2016 · 14 comments
Labels

Comments

@boneskull
Copy link
Contributor

Contents of scripts:

scripts/
├── bump
│   ├── index.sh
│   ├── major.sh
│   ├── minor.sh
│   └── patch.sh
├── preversion.sh
├── publish-please.sh
├── test
│   ├── browser
│   │   ├── dev.sh
│   │   └── index.sh
│   ├── index.sh
│   ├── lint.sh
│   ├── node.sh
│   └── nsp.sh
└── update.sh

test/index.sh invokes npm run test:lint && npm run test:nsp && npm run test:node && npm run test:browser. test/browser/index.sh invokes karma.

scripty erroneously executes test/browser/dev.sh. at least.. I think that's in error.

@searls
Copy link
Member

searls commented Apr 16, 2016

Thanks for this load test.

@searls
Copy link
Member

searls commented Apr 16, 2016

Just to clarify:

  1. "test:browser": "scripty" is in your package.json, confirm?
  2. When you run npm run test:browser, both scripts/test/browser/dev.sh and scripts/test/browser/index.sh are being run, right?

What if you rename index.sh to index? If that fixes it then the lib/resolve-script/generate-glob module is probably to blame

@searls searls added the bug label Apr 16, 2016
@searls
Copy link
Member

searls commented Apr 16, 2016

As always I'd appreciate any help you can offer, whether it's a way to reproduce the issue, a test, or a PR. Thanks

@boneskull
Copy link
Contributor Author

"test:browser": "scripty" is in your package.json, confirm?

yes

When you run npm run test:browser, both scripts/test/browser/dev.sh and scripts/test/browser/index.sh are being run, right?

yes

What if you rename index.sh to index? If that fixes it then the lib/resolve-script/generate-glob module is probably to blame

This fixes the issue.

@boneskull
Copy link
Contributor Author

in generate-glob you could probably leverage the glob module to avoid both fs.statSync() calls, and fix this.

@boneskull
Copy link
Contributor Author

...but that may not be any faster. sounds silly to generate a globspec using glob

@boneskull
Copy link
Contributor Author

boneskull commented Apr 16, 2016

also: ensure index is not a directory 😄

looks like you have that covered

@boneskull
Copy link
Contributor Author

this line should probably be

return path.join(expanded, '*')

the following

return path.join(expanded, '/') + '*'

is not windows-safe, afaik. the purpose of join of course is to insert the proper directory separators for you

@boneskull
Copy link
Contributor Author

return fs.statSync(f + '/index').isFile()

isn't windows-safe either, right? probably want

return fs.statSync(path.join(f, 'index')).isFile()

but you may blow that away if you change your strategy anyway

@boneskull
Copy link
Contributor Author

boneskull commented Apr 16, 2016

I think you can do the index check before you do the dir check too.

this might do it:

// untested
module.exports = function (dir1, dir2) {
  var expanded = path.resolve(dir1, path.join.apply(this, dir2.split(':')))
  // find any index files beneath expanded.  if it's not a directory,
  // this will fail
  var indexGlob = path.join(expanded, 'index*')
  if (glob.sync(indexGlob, {nodir: true}).length) {
    return indexGlob
  }
  // check if expanded is actually a directory
  var dirGlob = path.join(expanded, path.sep)
  if (glob.sync(dirGlob).length) {
    return dirGlob + '*'
  }
  // otherwise, default
  return expanded + '*'
}

@searls
Copy link
Member

searls commented Apr 16, 2016

A couple questions:

  1. Are you on Windows?
  2. Are you able to reproduce with a test? I tried last night and failed

@searls
Copy link
Member

searls commented Apr 16, 2016

Nevermind, was able to reproduce

searls added a commit that referenced this issue Apr 16, 2016
Need to:
* Refactor names
* Rewrite generate-globs unit test now that it does a different thing

Pretty big refactor to fix #20

* Move unit-test-specific fixtures into their own dir to avoid confusion
* Demarcate unit-test fixtures from one another so they don't become
tangled with one another and can be changed/deleted safely
* Write a "multi-glob" module which will accept multiple globbing
patterns and return the first hit. This allows us to avoid the stat
calls or moving glob.sync calls elsewhere
searls added a commit that referenced this issue Apr 16, 2016
Need to:

* Rewrite generate-globs unit test now that it does a different thing

Pretty big refactor to fix #20

* Refactor a bunch of names
* Move unit-test-specific fixtures into their own dir to avoid confusion
* Demarcate unit-test fixtures from one another so they don't become
tangled with one another and can be changed/deleted safely
* Write a "multi-glob" module which will accept multiple globbing
patterns and return the first hit. This allows us to avoid the stat
calls or moving glob.sync calls elsewhere
@boneskull
Copy link
Contributor Author

I'm not on Windows.

@searls searls closed this as completed in d3a1420 Apr 16, 2016
@searls
Copy link
Member

searls commented Apr 16, 2016

Thanks for your help with this. Fix landed in 1.2.3

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

No branches or pull requests

2 participants