From 99b7c6abfb588b79c648f22fab509ddbb731bd9d Mon Sep 17 00:00:00 2001 From: Kevin Gibbons Date: Sat, 12 Aug 2023 00:01:26 -0700 Subject: [PATCH 1/2] remove --js-out/--css-out; replace with --assets-dir --- package.json | 2 +- spec/index.html | 5 +- src/Spec.ts | 227 ++++++++++-------- src/args.ts | 31 ++- src/cli.ts | 24 +- src/ecmarkup.ts | 5 +- .../multipage.html/multipage/index.html | 2 +- .../multipage.html/multipage/second.html | 2 +- .../multipage.html/multipage/third.html | 2 +- 9 files changed, 165 insertions(+), 135 deletions(-) diff --git a/package.json b/package.json index c34f637c..e69c2ad9 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,7 @@ "scripts": { "build": "tsc -sourceMap", "build-release": "tsc", - "build-spec": "mkdir -p docs && node bin/ecmarkup.js spec/index.html docs/index.html --css-out docs/elements.css --js-out docs/ecmarkup.js", + "build-spec": "mkdir -p docs && node bin/ecmarkup.js spec/index.html docs/index.html --assets-dir=docs", "prepack": "safe-publish-latest && npm run build-release", "format-spec": "node bin/emu-format.js --write spec/index.html", "test": "mocha", diff --git a/spec/index.html b/spec/index.html index be688175..e12804b2 100644 --- a/spec/index.html +++ b/spec/index.html @@ -50,9 +50,8 @@

Options

`--watch`Rebuild when files change. `--verbose`Print verbose logging info. `--write-biblio`Emit a biblio file to the specified path. - `--assets``assets`"none" for no CSS/JS, "inline" for inline CSS/JS. - `--css-out``cssOut`Emit the Ecmarkup CSS file to the specified path. - `--js-out``jsOut`Emit the Ecmarkup JS file to the specified path. + `--assets``assets`"none" for no CSS/JS/etc, "inline" for inline CSS/JS/etc, "external" for external CSS/JS/etc. + `--assets-dir``assetsDir`Directory in which to place assets when using `--assets=external`. `--lint-spec``lintSpec`Enforce some style and correctness checks. `--error-formatter`The eslint formatter to be used for printing warnings and errors when using `--verbose`. Either the name of a built-in eslint formatter or the package name of an installed eslint compatible formatter. `--strict`Exit with an error if there are warnings. Cannot be used with `--watch`. diff --git a/src/Spec.ts b/src/Spec.ts index 81239bd9..df4bd53c 100644 --- a/src/Spec.ts +++ b/src/Spec.ts @@ -298,6 +298,7 @@ export function maybeAddClauseToEffectWorklist( export default class Spec { spec: this; opts: Options; + assets: { type: 'none' } | { type: 'inline' } | { type: 'external'; directory: string }; rootPath: string; rootDir: string; sourceText: string; @@ -340,12 +341,10 @@ export default class Spec { rootPath: string, fetch: (file: string, token: CancellationToken) => PromiseLike, dom: JSDOM, - opts: Options, + opts: Options = {}, sourceText: string, token = CancellationToken.none ) { - opts = opts || {}; - this.spec = this; this.opts = {}; this.rootPath = rootPath; @@ -364,6 +363,7 @@ export default class Spec { this.cancellationToken = token; this.generatedFiles = new Map(); this.log = opts.log ?? (() => {}); + // TODO warnings should probably default to console.error, with a reasonable formatting this.warn = opts.warn ? wrapWarn(sourceText, this, opts.warn) : () => {}; this._figureCounts = { table: 0, @@ -383,18 +383,45 @@ export default class Spec { this.processMetadata(); Object.assign(this.opts, opts); - if (this.opts.multipage) { - if (this.opts.jsOut || this.opts.cssOut) { - throw new Error('Cannot use --multipage with --js-out or --css-out'); - } + if (this.opts.jsOut || this.opts.cssOut) { + throw new Error('--js-out and --css-out have been removed; use --assets-dir instead'); + } - if (this.opts.outfile == null) { - this.opts.outfile = ''; - } + if ( + this.opts.assets != null && + this.opts.assets !== 'external' && + this.opts.assetsDir != null + ) { + throw new Error(`--assets=${this.opts.assets} cannot be used --assets-dir"`); + } - if (this.opts.assets !== 'none') { - this.opts.jsOut = path.join(this.opts.outfile, 'ecmarkup.js'); - this.opts.cssOut = path.join(this.opts.outfile, 'ecmarkup.css'); + if (this.opts.multipage) { + this.opts.outfile ??= ''; + const type = this.opts.assets ?? 'external'; + if (type === 'inline') { + throw new Error('assets cannot be inline for multipage builds'); + } else if (type === 'none') { + this.assets = { type: 'none' }; + } else { + this.assets = { + type: 'external', + directory: this.opts.assetsDir ?? path.join(this.opts.outfile, 'assets'), + }; + } + } else { + const type = this.opts.assets ?? (this.opts.assetsDir == null ? 'inline' : 'external'); + if (type === 'inline') { + console.error('Warning: inlining assets; this should not be done for production builds.'); + this.assets = { type: 'inline' }; + } else if (type === 'none') { + this.assets = { type: 'none' }; + } else { + this.assets = { + type: 'external', + directory: + this.opts.assetsDir ?? + (this.opts.outfile ? path.join(path.dirname(this.opts.outfile), 'assets') : 'assets'), + }; } } @@ -562,12 +589,12 @@ export default class Spec { (await concatJs(sdoJs, tocJs)) + `\n;let usesMultipage = ${!!this.opts.multipage}`; const jsSha = sha(jsContents); + await this.buildAssets(jsContents, jsSha); + if (this.opts.multipage) { - await this.buildMultipage(wrapper, commonEles, jsSha); + await this.buildMultipage(wrapper, commonEles); } - await this.buildAssets(jsContents, jsSha); - const file = this.opts.multipage ? path.join(this.opts.outfile!, 'index.html') : this.opts.outfile ?? null; @@ -1004,7 +1031,7 @@ export default class Spec { return null; } - private async buildMultipage(wrapper: Element, commonEles: Element[], jsSha: string) { + private async buildMultipage(wrapper: Element, commonEles: Element[]) { let stillIntro = true; const introEles = []; const sections: { name: string; eles: Element[] }[] = []; @@ -1101,53 +1128,46 @@ export default class Spec { } const head = this.doc.head.cloneNode(true) as HTMLHeadElement; - this.addStyle(head, 'ecmarkup.css'); // omit `../` because we rewrite `` elements below - this.addStyle( - head, - `https://cdnjs.cloudflare.com/ajax/libs/highlight.js/${ - (hljs as any).versionString - }/styles/base16/solarized-light.min.css` - ); - - const script = this.doc.createElement('script'); - script.src = '../ecmarkup.js?cache=' + jsSha; - script.setAttribute('defer', ''); - head.appendChild(script); const containedMap = JSON.stringify(Object.fromEntries(sectionToContainedIds)).replace( /[\\`$]/g, '\\$&' ); - const multipageJsContents = `'use strict'; + if (this.assets.type !== 'none') { + const multipageJsContents = `'use strict'; let multipageMap = JSON.parse(\`${containedMap}\`); ${await utils.readFile(path.join(__dirname, '../js/multipage.js'))} `; - if (this.opts.assets !== 'none') { - this.generatedFiles.set( - path.join(this.opts.outfile!, 'multipage/multipage.js'), - multipageJsContents - ); - } - const multipageScript = this.doc.createElement('script'); - multipageScript.src = 'multipage.js?cache=' + sha(multipageJsContents); - multipageScript.setAttribute('defer', ''); - head.insertBefore(multipageScript, head.querySelector('script')); + // assets are never internal for multipage builds + // @ts-expect-error + const multipageLocationOnDisk = path.join(this.assets.directory, 'multipage.js'); + + this.generatedFiles.set(multipageLocationOnDisk, multipageJsContents); + + // the path will be rewritten below + // so it should initially be relative to outfile, not outfile/multipage + const multipageScript = this.doc.createElement('script'); + multipageScript.src = + path.relative(this.opts.outfile!, multipageLocationOnDisk) + + '?cache=' + + sha(multipageJsContents); + multipageScript.setAttribute('defer', ''); + head.insertBefore(multipageScript, head.querySelector('script')); + } for (const { name, eles } of sections) { this.log(`Generating section ${name}...`); const headClone = head.cloneNode(true) as HTMLHeadElement; const commonClone = commonEles.map(e => e.cloneNode(true)); const clones = eles.map(e => e.cloneNode(true)); - const allClones = [headClone, ...commonClone, ...clones]; - // @ts-ignore - const links = allClones.flatMap(e => [...e.querySelectorAll('a,link')]); - for (const link of links) { - if (linkIsAbsolute(link)) { + const allClones = [headClone, ...commonClone, ...clones] as Element[]; + for (const anchor of allClones.flatMap(e => [...e.querySelectorAll('a')])) { + if (linkIsAbsolute(anchor)) { continue; } - if (linkIsInternal(link)) { - let p = link.hash.substring(1); + if (linkIsInternal(anchor)) { + let p = anchor.hash.substring(1); if (!containedIdToSection.has(p)) { try { p = decodeURIComponent(p); @@ -1158,29 +1178,41 @@ ${await utils.readFile(path.join(__dirname, '../js/multipage.js'))} this.warn({ type: 'node', ruleId: 'multipage-link-target', - message: 'could not find appropriate section for ' + link.hash, - node: link, + message: 'could not find appropriate section for ' + anchor.hash, + node: anchor, }); continue; } } const targetSec = containedIdToSection.get(p)!; - link.href = (targetSec === 'index' ? './' : targetSec + '.html') + link.hash; - } else if (linkIsPathRelative(link)) { - link.href = '../' + pathFromRelativeLink(link); + anchor.href = (targetSec === 'index' ? './' : targetSec + '.html') + anchor.hash; + } else if (linkIsPathRelative(anchor)) { + anchor.href = path.relative('multipage', pathFromRelativeLink(anchor)); } } - // @ts-ignore + + for (const link of allClones.flatMap(e => [...e.querySelectorAll('link')])) { + if (!linkIsAbsolute(link) && linkIsPathRelative(link)) { + link.href = path.relative('multipage', pathFromRelativeLink(link)); + } + } + for (const img of allClones.flatMap(e => [...e.querySelectorAll('img')])) { if (!/^(http:|https:|:|\/)/.test(img.src)) { - img.src = '../' + img.src; + img.src = path.relative('multipage', img.src); + } + } + + for (const script of allClones.flatMap(e => [...e.querySelectorAll('script')])) { + if (script.src != null && !/^(http:|https:|:|\/)/.test(script.src)) { + script.src = path.relative('multipage', script.src); } } + // prettier-ignore - // @ts-ignore - for (const object of allClones.flatMap(e => [...e.querySelectorAll('object[data]')])) { + for (const object of allClones.flatMap(e => [...e.querySelectorAll('object[data]')]) as HTMLObjectElement[]) { if (!/^(http:|https:|:|\/)/.test(object.data)) { - object.data = '../' + object.data; + object.data = path.relative('multipage', object.data); } } @@ -1191,9 +1223,9 @@ ${await utils.readFile(path.join(__dirname, '../js/multipage.js'))} headClone.appendChild(canonical); } - // @ts-ignore + // @ts-expect-error const commonHTML = commonClone.map(e => e.outerHTML).join('\n'); - // @ts-ignore + // @ts-expect-error const clonesHTML = clones.map(e => e.outerHTML).join('\n'); const content = `${htmlEle}\n${headClone.outerHTML}\n${commonHTML}
${clonesHTML}
`; @@ -1202,63 +1234,50 @@ ${await utils.readFile(path.join(__dirname, '../js/multipage.js'))} } private async buildAssets(jsContents: string, jsSha: string) { - const cssContents = await utils.readFile(path.join(__dirname, '../css/elements.css')); + if (this.assets.type === 'none') return; - if (this.opts.jsOut) { - this.generatedFiles.set(this.opts.jsOut, jsContents); + // check for very old manual 'ecmarkup.js'/'ecmarkup.css' + const oldEles = this.doc.querySelectorAll( + "script[src='ecmarkup.js'],link[href='ecmarkup.css']" + ); + for (const item of oldEles) { + this.warn({ + type: 'attr', + ruleId: 'old-script-or-style', + node: item, + attr: item.tagName === 'SCRIPT' ? 'src' : 'href', + message: + 'ecmarkup will insert its own js/css; the input document should not include tags for them', + }); } - if (this.opts.cssOut) { - this.generatedFiles.set(this.opts.cssOut, cssContents); - } + const cssContents = await utils.readFile(path.join(__dirname, '../css/elements.css')); + if (this.assets.type === 'external') { + const outDir = this.opts.outfile + ? this.opts.multipage + ? this.opts.outfile + : path.dirname(this.opts.outfile) + : process.cwd(); - if (this.opts.assets === 'none') return; + const scriptLocationOnDisk = path.join(this.assets.directory, 'ecmarkup.js'); + const styleLocationOnDisk = path.join(this.assets.directory, 'ecmarkup.css'); - const outDir = this.opts.outfile - ? this.opts.multipage - ? this.opts.outfile - : path.dirname(this.opts.outfile) - : process.cwd(); + this.generatedFiles.set(scriptLocationOnDisk, jsContents); + this.generatedFiles.set(styleLocationOnDisk, cssContents); - if (this.opts.jsOut) { - let skipJs = false; - const scripts = this.doc.querySelectorAll('script'); - for (let i = 0; i < scripts.length; i++) { - const script = scripts[i]; - const src = script.getAttribute('src'); - if (src && path.normalize(path.join(outDir, src)) === path.normalize(this.opts.jsOut)) { - this.log(`Found existing js link to ${src}, skipping inlining...`); - skipJs = true; - } - } - if (!skipJs) { - const script = this.doc.createElement('script'); - script.src = path.relative(outDir, this.opts.jsOut) + '?cache=' + jsSha; - script.setAttribute('defer', ''); - this.doc.head.appendChild(script); - } + const script = this.doc.createElement('script'); + script.src = path.relative(outDir, scriptLocationOnDisk) + '?cache=' + jsSha; + script.setAttribute('defer', ''); + this.doc.head.appendChild(script); + + this.addStyle(this.doc.head, path.relative(outDir, styleLocationOnDisk)); } else { + // i.e. assets.type === 'inline' this.log('Inlining JavaScript assets...'); const script = this.doc.createElement('script'); script.textContent = jsContents; this.doc.head.appendChild(script); - } - if (this.opts.cssOut) { - let skipCss = false; - const links = this.doc.querySelectorAll('link[rel=stylesheet]'); - for (let i = 0; i < links.length; i++) { - const link = links[i]; - const href = link.getAttribute('href'); - if (href && path.normalize(path.join(outDir, href)) === path.normalize(this.opts.cssOut)) { - this.log(`Found existing css link to ${href}, skipping inlining...`); - skipCss = true; - } - } - if (!skipCss) { - this.addStyle(this.doc.head, path.relative(outDir, this.opts.cssOut)); - } - } else { this.log('Inlining CSS assets...'); const style = this.doc.createElement('style'); style.textContent = cssContents; diff --git a/src/args.ts b/src/args.ts index 2abd4046..5254645e 100644 --- a/src/args.ts +++ b/src/args.ts @@ -28,20 +28,16 @@ export const options = [ { name: 'assets', type: String, - typeLabel: 'none|inline|external', // TODO I don't think we actually distinguish between inline and external - description: 'Link to css and js assets (default: inline, unless --multipage)', - }, - { - name: 'css-out', - type: String, - typeLabel: '{underline file}', - description: 'Path to a file where the CSS assets should be written', + typeLabel: 'none|inline|external', + description: + 'Omit assets, inline them, or add them as external. Default: inline, unless --multipage or --assets-dir are specified, in which case external.', }, { - name: 'js-out', + name: 'assets-dir', type: String, - typeLabel: '{underline file}', - description: 'Path to a file where the JS assets should be written', + typeLabel: '{underline dir}', + description: + 'The directory in which to place generated assets when using --assets=external. Implies --assets=external. Defaults to [outfile]/assets.', }, { name: 'no-toc', @@ -74,8 +70,7 @@ export const options = [ { name: 'multipage', type: Boolean, - description: - 'Generate a multipage version of the spec. Cannot be used with --js-out or --css-out.', + description: 'Generate a multipage version of the spec. Implies --assets=external.', }, { name: 'strict', @@ -99,4 +94,14 @@ export const options = [ multiple: true, defaultOption: true, }, + + // removed; still defined here so we can give better errors + { + name: 'css-out', + type: String, + }, + { + name: 'js-out', + type: String, + }, ] as const; diff --git a/src/cli.ts b/src/cli.ts index d7099cd5..b0d481c3 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -24,7 +24,7 @@ function usage() { }, { header: 'Options', - hide: ['files'], + hide: ['files', 'js-out', 'css-out'], optionList: options as unknown as commandLineUsage.OptionDefinition[], }, ]) @@ -64,18 +64,21 @@ if (args.strict && args.watch) { fail('Cannot use --strict with --watch'); } +if (args['js-out'] || args['css-out']) { + fail('--js-out and --css-out have been removed; specify --assets-dir instead'); +} + if (args.assets != null && !['none', 'inline', 'external'].includes(args.assets)) { - fail('--assets requires "none", "inline", or "external"'); + fail('--assets must be "none", "inline", or "external"'); } -// TODO just drop these -if (!outfile && (args['js-out'] || args['css-out'])) { - fail('When using --js-out or --css-out you must specify an output file'); +if (args.assets != null && args.assets !== 'external' && args['assets-dir'] != null) { + fail(`--assets=${args.assets} cannot be used --assets-dir"`); } if (args.multipage) { - if (args['js-out'] || args['css-out']) { - fail('Cannot use --multipage with --js-out or --css-out'); + if (args.assets != null && !['none', 'inline'].includes(args.assets)) { + fail('--multipage implies --assets=external'); } if (!outfile) { @@ -97,8 +100,6 @@ const build = debounce(async function build() { const opts: Options = { multipage: args.multipage, outfile, - jsOut: args['js-out'], - cssOut: args['css-out'], extraBiblios: [], lintSpec: !!args['lint-spec'], }; @@ -114,9 +115,14 @@ const build = debounce(async function build() { if (args['old-toc'] != null) { opts.oldToc = args['old-toc']; } + if (args.assets != null) { opts.assets = args.assets as 'none' | 'inline' | 'external'; } + if (args['assets-dir'] != null) { + opts.assetsDir = args['assets-dir']; + } + let warned = false; for (let toResolve of args['load-biblio'] ?? []) { diff --git a/src/ecmarkup.ts b/src/ecmarkup.ts index 4bd7c091..379d54a4 100644 --- a/src/ecmarkup.ts +++ b/src/ecmarkup.ts @@ -38,9 +38,10 @@ export interface Options { oldToc?: boolean; markEffects?: boolean; lintSpec?: boolean; - cssOut?: string; - jsOut?: string; + cssOut?: never; + jsOut?: never; assets?: 'none' | 'inline' | 'external'; + assetsDir?: string; outfile?: string; boilerplate?: Boilerplate; log?: (msg: string) => void; diff --git a/test/baselines/generated-reference/multipage.html/multipage/index.html b/test/baselines/generated-reference/multipage.html/multipage/index.html index ce8fbf01..45598d1f 100644 --- a/test/baselines/generated-reference/multipage.html/multipage/index.html +++ b/test/baselines/generated-reference/multipage.html/multipage/index.html @@ -1,7 +1,7 @@ - +