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

Fix path resolution of the directory whose name included glob special chars #230

Merged
merged 3 commits into from
Jun 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- Add a trailing slash to the directory links on server index page to avoid broken path resolution ([#221](https://github.com/marp-team/marp-cli/pull/221) by [@n-ari](https://github.com/n-ari))
- Restart CSS animations when switching page in bespoke template ([#222](https://github.com/marp-team/marp-cli/pull/222))
- Fix path resolution of the directory whose name included glob special chars ([#227](https://github.com/marp-team/marp-cli/issues/227), [#230](https://github.com/marp-team/marp-cli/pull/230))

### Changed

Expand Down
17 changes: 10 additions & 7 deletions src/file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ export class File {
): Promise<string[]> {
const filepaths = new Set<string>()
const globs: string[] = []
const dirs: string[] = []

// Collect passed files that refers to a real path at first
for (const p of paths) {
Expand All @@ -176,6 +177,9 @@ export class File {
if (s.isFile()) {
filepaths.add(path.resolve(p))
continue
} else if (s.isDirectory()) {
dirs.push(path.resolve(p))
continue
}
} catch (e) {}

Expand All @@ -184,13 +188,12 @@ export class File {
}

// Find remaining path through globby
;(
await globby(globs, {
absolute: true,
ignore: ['**/node_modules'],
...opts,
})
).forEach((p) => filepaths.add(p))
const gOpts = { absolute: true, ignore: ['**/node_modules'], ...opts }
;(await globby(globs, gOpts)).forEach((p) => filepaths.add(p))

for (const cwd of dirs) {
;(await globby('.', { cwd, ...gOpts })).forEach((p) => filepaths.add(p))
Copy link
Member Author

Choose a reason for hiding this comment

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

Expanding file patterns won't work in escaped glob path so we are using cwd option to specify the base directory.

}

return [...filepaths.values()].map((p) => path.normalize(p))
}
Expand Down
Empty file added test/_files/(sp)/!.md
Empty file.
29 changes: 26 additions & 3 deletions test/marp-cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ describe('Marp CLI', () => {
jest.spyOn(cli, 'info').mockImplementation()

expect(await marpCli(['--input-dir', files])).toBe(0)
expect(writeFile).toHaveBeenCalledTimes(5)
expect(writeFile).toHaveBeenCalledTimes(6)
writeFile.mock.calls.forEach(([fn]) => expect(fn).toMatch(/\.html$/))
})

Expand Down Expand Up @@ -246,14 +246,23 @@ describe('Marp CLI', () => {
)
})

context('when the directory path has included glob pattern', () => {
it('converts files in specified dir', async () => {
jest.spyOn(cli, 'info').mockImplementation()

expect(await marpCli(['--input-dir', assetFn('_files/(sp)')])).toBe(0)
expect(writeFile).toHaveBeenCalledTimes(1)
})
})

context('when the output folder is specified by -o option', () => {
it('converts markdowns with keeping folder structure', async () => {
const args = ['--input-dir', files, '-o', assetFn('dist')]

jest.spyOn(cli, 'info').mockImplementation()

expect(await marpCli(args)).toBe(0)
expect(writeFile).toHaveBeenCalledTimes(5)
expect(writeFile).toHaveBeenCalledTimes(6)

const outputFiles = writeFile.mock.calls.map(([fn]) => fn)
expect(outputFiles).toContain(assetFn('dist/1.html'))
Expand Down Expand Up @@ -763,7 +772,7 @@ describe('Marp CLI', () => {

expect(await marpCli([assetFn('_files')])).toBe(0)
expect(cliInfo.mock.calls.map(([m]) => m)).toContainEqual(
expect.stringContaining('5 markdowns')
expect.stringContaining('6 markdowns')
)
})

Expand All @@ -789,6 +798,20 @@ describe('Marp CLI', () => {
})
})

context(
'when glob special chars are included in real directory path',
() => {
it('finds out markdown files in specified directory correctly', async () => {
jest.spyOn(cli, 'info').mockImplementation()
jest
.spyOn<any, any>(Converter.prototype, 'convertFiles')
.mockImplementation(() => [])

expect(await marpCli([assetFn('_files/(sp)')])).toBe(0)
})
}
)

context('with --server option', () => {
it('treats passed directory as an input directory of the server', async () => {
jest.spyOn(cli, 'info').mockImplementation()
Expand Down
4 changes: 2 additions & 2 deletions test/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,8 @@ describe('Server', () => {

const $ = cheerio.load(response.text)
expect($('h1').text()).toBe('/')
expect($('ul#index li')).toHaveLength(8) // Actual file count
expect($('ul#index li.directory')).toHaveLength(4) // Directories
expect($('ul#index li')).toHaveLength(9) // Actual file count
expect($('ul#index li.directory')).toHaveLength(5) // Directories
expect($('ul#index li.directory.nodeModules')).toHaveLength(1)
expect($('ul#index li.convertible')).toHaveLength(3) // Markdown files

Expand Down