Skip to content

Commit f78377a

Browse files
luis-almeidaluis-almeida
authored
Merge pull request #369 from zendesk/luis/themes_preview_bug_fixes
fix(themes): several fixes for themes:preview and themes:migrate
2 parents f39dcac + 264ef00 commit f78377a

12 files changed

Lines changed: 118 additions & 33 deletions

‎packages/zcli-themes/src/commands/themes/migrate.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,9 @@ export default class Migrate extends Command {
1818
static strict = false
1919

2020
async run () {
21-
const { flags, argv: [themeDirectory] } = await this.parse(Migrate)
21+
const { argv: [themeDirectory] } = await this.parse(Migrate)
2222
const themePath = path.resolve(themeDirectory)
2323

24-
await migrate(themePath, flags)
24+
await migrate(themePath)
2525
}
2626
}

‎packages/zcli-themes/src/commands/themes/preview.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ export default class Preview extends Command {
9999
`${themePath}/style.css`
100100
]
101101

102-
const watcher = chokidar.watch(monitoredPaths).on('change', async (path) => {
102+
const handleThemeChange = async (path: string) => {
103103
this.log(chalk.bold('Change'), path)
104104
try {
105105
await preview(themePath, flags)
@@ -111,7 +111,12 @@ export default class Preview extends Command {
111111
} catch (e) {
112112
this.error(e as Error, { exit: false })
113113
}
114-
})
114+
}
115+
116+
const watcher = chokidar.watch(monitoredPaths, { ignoreInitial: true })
117+
.on('add', handleThemeChange)
118+
.on('change', handleThemeChange)
119+
.on('unlink', handleThemeChange)
115120

116121
return {
117122
close: () => {

‎packages/zcli-themes/src/lib/getAssets.test.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,30 @@ describe('getAssets', () => {
3939
])
4040
})
4141

42+
it('returns basenames as urls when flags are not provided', () => {
43+
const existsSyncStub = sinon.stub(fs, 'existsSync')
44+
const readdirSyncStub = sinon.stub(fs, 'readdirSync')
45+
46+
existsSyncStub
47+
.withArgs('theme/path/assets')
48+
.returns(true)
49+
50+
readdirSyncStub.returns(['foo.png', 'bar.png'] as any)
51+
52+
const assets = getAssets('theme/path')
53+
54+
expect(assets).to.deep.equal([
55+
[
56+
{ base: 'foo.png', dir: '', ext: '.png', name: 'foo', root: '' },
57+
'foo.png'
58+
],
59+
[
60+
{ base: 'bar.png', dir: '', ext: '.png', name: 'bar', root: '' },
61+
'bar.png'
62+
]
63+
])
64+
})
65+
4266
it('throws an error when an asset has illegal characters in its name', () => {
4367
const existsSyncStub = sinon.stub(fs, 'existsSync')
4468
const readdirSyncStub = sinon.stub(fs, 'readdirSync')

‎packages/zcli-themes/src/lib/getAssets.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import * as fs from 'fs'
44
import * as path from 'path'
55
import { getLocalServerBaseUrl } from './getLocalServerBaseUrl'
66

7-
export default function getAssets (themePath: string, flags: Flags): [path.ParsedPath, string][] {
7+
export default function getAssets (themePath: string, flags?: Flags): [path.ParsedPath, string][] {
88
const assetsPath = `${themePath}/assets`
99
const filenames = fs.existsSync(assetsPath) ? fs.readdirSync(assetsPath) : []
1010
const assets: [path.ParsedPath, string][] = []
@@ -18,7 +18,8 @@ export default function getAssets (themePath: string, flags: Flags): [path.Parse
1818
)
1919
}
2020
if (!name.startsWith('.')) {
21-
assets.push([parsedPath, `${getLocalServerBaseUrl(flags)}/guide/assets/${filename}`])
21+
const url = flags ? `${getLocalServerBaseUrl(flags)}/guide/assets/${filename}` : filename
22+
assets.push([parsedPath, url])
2223
}
2324
})
2425

‎packages/zcli-themes/src/lib/getVariables.test.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,23 @@ describe('getVariables', () => {
4040
])
4141
})
4242

43+
it('uses the matched filename as the value when flags are not provided', () => {
44+
const existsSyncStub = sinon.stub(fs, 'existsSync')
45+
const readdirSyncStub = sinon.stub(fs, 'readdirSync')
46+
47+
existsSyncStub
48+
.withArgs('theme/path/settings')
49+
.returns(true)
50+
51+
readdirSyncStub.returns(['logo.png', 'favicon.png'] as any)
52+
53+
expect(getVariables('theme/path', settings)).to.deep.equal([
54+
{ identifier: 'color', type: 'color', value: '#999' },
55+
{ identifier: 'logo', type: 'file', value: 'logo.png' },
56+
{ identifier: 'favicon', type: 'file', value: 'favicon.png' }
57+
])
58+
})
59+
4360
it('throws an error when it doesn\'t find an asset within the settings folder for a variable of type "file"', () => {
4461
const existsSyncStub = sinon.stub(fs, 'existsSync')
4562
const readdirSyncStub = sinon.stub(fs, 'readdirSync')

‎packages/zcli-themes/src/lib/getVariables.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import * as path from 'path'
44
import { CLIError } from '@oclif/core/lib/errors'
55
import { getLocalServerBaseUrl } from './getLocalServerBaseUrl'
66

7-
export default function getVariables (themePath: string, settings: Setting[], flags: Flags): Variable[] {
7+
export default function getVariables (themePath: string, settings: Setting[], flags?: Flags): Variable[] {
88
const settingsPath = `${themePath}/settings`
99
const filenames = fs.existsSync(settingsPath) ? fs.readdirSync(settingsPath) : []
1010

@@ -18,7 +18,7 @@ export default function getVariables (themePath: string, settings: Setting[], fl
1818
`The setting "${variable.identifier}" of type "file" does not have a matching file within the "settings" folder`
1919
)
2020
}
21-
variable.value = file && `${getLocalServerBaseUrl(flags)}/guide/settings/${file}`
21+
variable.value = flags ? `${getLocalServerBaseUrl(flags)}/guide/settings/${file}` : file
2222
}
2323
return variable
2424
})

‎packages/zcli-themes/src/lib/migrate.test.ts‎

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,6 @@ const manifest = {
2727
]
2828
}
2929

30-
const flags = {
31-
bind: 'localhost',
32-
port: 1000,
33-
logs: true,
34-
livereload: false
35-
}
36-
3730
describe('migrate', () => {
3831
beforeEach(() => {
3932
sinon.restore()
@@ -56,16 +49,16 @@ describe('migrate', () => {
5649
'custom_pages/faq': '<h1>FAQ</h1>'
5750
})
5851

59-
getVariablesStub.withArgs('theme/path', manifest.settings, flags).returns([
52+
getVariablesStub.withArgs('theme/path', manifest.settings).returns([
6053
{ identifier: 'color', type: 'color', value: '#999' },
6154
{
6255
identifier: 'logo',
6356
type: 'file',
64-
value: 'http://localhost:1000/guide/settings/logo.png'
57+
value: 'logo.png'
6558
}
6659
])
6760

68-
getAssetsStub.withArgs('theme/path', flags).returns([
61+
getAssetsStub.withArgs('theme/path').returns([
6962
[
7063
{
7164
base: 'background.png',
@@ -74,7 +67,7 @@ describe('migrate', () => {
7467
name: 'background',
7568
root: ''
7669
},
77-
'http://localhost:1000/guide/assets/background.png'
70+
'background.png'
7871
]
7972
])
8073

@@ -98,7 +91,7 @@ describe('migrate', () => {
9891
}) as axios.AxiosPromise
9992
)
10093

101-
await migrate('theme/path', flags)
94+
await migrate('theme/path')
10295

10396
expect(
10497
requestStub.calledWith(
@@ -114,12 +107,11 @@ describe('migrate', () => {
114107
'article_pages/product_updates': '<h1>Product updates</h1>',
115108
'custom_pages/faq': '<h1>FAQ</h1>',
116109
assets: {
117-
'background.png':
118-
'http://localhost:1000/guide/assets/background.png'
110+
'background.png': 'background.png'
119111
},
120112
variables: {
121113
color: '#999',
122-
logo: 'http://localhost:1000/guide/settings/logo.png'
114+
logo: 'logo.png'
123115
},
124116
metadata: { api_version: 1 }
125117
}
@@ -178,7 +170,7 @@ describe('migrate', () => {
178170
const errorStub = sinon.stub(errors, 'error').callThrough()
179171

180172
try {
181-
await migrate('theme/path', flags)
173+
await migrate('theme/path')
182174
} catch {
183175
const [call] = errorStub.getCalls()
184176
const [error] = call.args
@@ -206,7 +198,7 @@ describe('migrate', () => {
206198
const errorStub = sinon.stub(errors, 'error').callThrough()
207199

208200
try {
209-
await migrate('theme/path', flags)
201+
await migrate('theme/path')
210202
} catch {
211203
const [call] = errorStub.getCalls()
212204
const [error] = call.args
@@ -230,7 +222,7 @@ describe('migrate', () => {
230222
const errorStub = sinon.stub(errors, 'error').callThrough()
231223

232224
try {
233-
await migrate('theme/path', flags)
225+
await migrate('theme/path')
234226
} catch {
235227
const [call] = errorStub.getCalls()
236228
const [error] = call.args
@@ -250,7 +242,7 @@ describe('migrate', () => {
250242
const errorStub = sinon.stub(errors, 'error').callThrough()
251243

252244
try {
253-
await migrate('theme/path', flags)
245+
await migrate('theme/path')
254246
} catch {
255247
const [call] = errorStub.getCalls()
256248
const [error] = call.args

‎packages/zcli-themes/src/lib/migrate.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { Flags, ValidationErrors } from '../types'
1+
import type { ValidationErrors } from '../types'
22
import getManifest from './getManifest'
33
import getTemplates from './getTemplates'
44
import getVariables from './getVariables'
@@ -14,11 +14,11 @@ import rewriteAssets from './rewriteAssets'
1414
import handleTemplateError from './handleTemplateError'
1515
import parseAxiosError from './parseAxiosError'
1616

17-
export default async function migrate (themePath: string, flags: Flags): Promise<string | void> {
17+
export default async function migrate (themePath: string): Promise<string | void> {
1818
const manifest = getManifest(themePath)
1919
const templates = getTemplates(themePath)
20-
const variables = getVariables(themePath, manifest.settings, flags)
21-
const assets = getAssets(themePath, flags)
20+
const variables = getVariables(themePath, manifest.settings)
21+
const assets = getAssets(themePath)
2222

2323
const variablesPayload = variables.reduce((payload, variable) => ({
2424
...payload,

‎packages/zcli-themes/src/lib/rewriteTemplates.test.ts‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ describe('rewriteTemplates', () => {
99
})
1010

1111
it('writes templates to the correct file paths', () => {
12+
sinon.stub(fs, 'mkdirSync')
1213
const writeFileSyncStub = sinon.stub(fs, 'writeFileSync')
1314

1415
const templates = {
@@ -35,6 +36,7 @@ describe('rewriteTemplates', () => {
3536
})
3637

3738
it('throws an error when file cannot be written', () => {
39+
sinon.stub(fs, 'mkdirSync')
3840
const writeFileSyncStub = sinon.stub(fs, 'writeFileSync')
3941

4042
writeFileSyncStub.throws(new Error('Permission denied'))
@@ -58,7 +60,8 @@ describe('rewriteTemplates', () => {
5860
expect(writeFileSyncStub.callCount).to.equal(0)
5961
})
6062

61-
it('handles nested template paths correctly', () => {
63+
it('creates intermediate directories for nested template paths', () => {
64+
const mkdirSyncStub = sinon.stub(fs, 'mkdirSync')
6265
const writeFileSyncStub = sinon.stub(fs, 'writeFileSync')
6366

6467
const templates = {
@@ -68,6 +71,15 @@ describe('rewriteTemplates', () => {
6871

6972
rewriteTemplates('theme/path', templates)
7073

74+
expect(mkdirSyncStub.callCount).to.equal(2)
75+
expect(mkdirSyncStub.firstCall.args).to.deep.equal([
76+
'theme/path/templates/article_pages',
77+
{ recursive: true }
78+
])
79+
expect(mkdirSyncStub.secondCall.args).to.deep.equal([
80+
'theme/path/templates/custom_pages/deep/nested',
81+
{ recursive: true }
82+
])
7183
expect(writeFileSyncStub.callCount).to.equal(2)
7284
expect(writeFileSyncStub.firstCall.args).to.deep.equal([
7385
'theme/path/templates/article_pages/product_updates.hbs',

‎packages/zcli-themes/src/lib/rewriteTemplates.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { CLIError } from '@oclif/core/lib/errors'
22
import * as fs from 'fs'
3+
import * as path from 'path'
34
import * as chalk from 'chalk'
45

56
export default function rewriteTemplates (themePath: string, templates: Record<string, string>) {
@@ -8,6 +9,7 @@ export default function rewriteTemplates (themePath: string, templates: Record<s
89

910
if (typeof content === 'string') {
1011
try {
12+
fs.mkdirSync(path.dirname(filePath), { recursive: true })
1113
fs.writeFileSync(filePath, content)
1214
} catch (error) {
1315
throw new CLIError(chalk.red(`Failed to write template file: ${filePath}`))

0 commit comments

Comments
 (0)