Drop dependency on CssWriter (discussion)

[JohnGurin]

Eliminate the catching of CssWriter in a scope around Rollup configuration and let Rollup handle files.

...

1)
Change generateBundle() to generateBundle(outputOptions, bundle)

2)
Replace css(writer); at the end of generateBundle with

// ...
// css(writer);

// outputOptions as the second argument allows css function
// to use the options for custom css file name
let cssFilename = css(writer, outputOptions)

// If css returns nothing then do nothing more
// Backward compatibility
if (!cssFilename)
    return

// If css returns true
//   if output is 'file', then derive css filename from 'file'
//   if output is 'dir', use 'name' option
if (cssFilename === true) {
    const {file, name} = outputOptions
    cssFilename = file ? path.basename(file).replace(/\.[^/.]+$/, '') + '.css'
                : name ? name + '.css'
                : ''
}

if (cssFilename === '')
    throw Error(`css file name is an empty string.\n` +
                `You should provide either 'file' or 'name' in output option,\n` +
                `or explicitly return file name from svelte.css function`);
if (bundle[cssFilename])
    throw Error(`css file name (${cssFilename}) overrides bundle's another chunk file name`);

let map
if (outputOptions.sourcemap) {

    // 1.1) use CssWriter functionality 
    // map = writer.map

    // 1.2) or create map inplace
    map = {             // I cannot figure out how to initiate map
        version: 3,     // as new SourceMap which is used in Rollup
        file: cssFilename,
        sources,
        sourcesContent,
        names: [],
        mappings: encode(mappings),
    };

    // For Rollup, map must have 'toUrl' for sourcemap === 'inline' or
    // 'toString' if sourcemap is not falsy
    // 'toUrl' and 'toString' could be added to 'this.map' in CssWriter constructor.
    // But it would be better to ditch CssWriter if the following
    // approaches solve the problem and use SourceMap class for map.
    
    // (addition) 2.1 does not work for sourcemap === 'inline'
    // handling css as js chunk results in wrong source map comment
    // Rollup adds '//# sourceMappingURL=' instead of '/*# .... */'
    map.toUrl = () => {
        const encoding = 'base64'
        return `data:application/json;charset=utf-8;${encoding},` +
            Buffer.from(map.mappings).toString(encoding)
    },
    map.toString = () => {
        return JSON.stringify(map)
    }
}

then use either of two approaches (as continuation of code above)

// 2.1)
// Adding css to a bundle as a chunk handles sourcemap automatically, but
// when output is 'file' instead of 'dir', Rollup throws
// "You must set "output.dir" instead of "output.file" when generating multiple chunks."
bundle[cssFilename] = {
    code: result,
    fileName: cssFilename,
    map,
    type: 'chunk' // if omitted, Rollup adds type 'asset' itself,
                  // in which case map processing is skipped
}

or

// 2.2)
// Or we can add both css and css.map as assets manually. I don't
// know whether it would affect other css plugins or smth. else
if (outputOptions.sourcemap === 'inline') {
    result += '\n/*# sourceMappingURL=' + map.toUrl() + ' */'

} else if (outputOptions.sourcemap) {
    const cssMapFilename = cssFilename + '.map'
    result += '\n/*# sourceMappingURL=' + cssMapFilename + ' */'
    if (bundle[cssMapFilename])
        throw Error(`css map file name (${cssMapFilename}) overrides ` +
                     `bundle's another chunk file name`);
    bundle[cssMapFilename] = {
        source: map.toString(),
        fileName: cssMapFilename,
    }
}

bundle[cssFilename] = {
    source: result,
    fileName: cssFilename,
}

With this modification svelte plugin adds its output to Rollup bundle.
I did not research what actually svelte plugin does in transform. I suppose the plugin should be capable to prepare css files for Rollup chunks (code splitting) in the ideal case, which it does not do for now.

[Conduitry]

Is this change intended to fix a particular bug or implement a particular feature? Is this suggesting some code cleanup?

[JohnGurin]

I would say, it's a little bit all three

Rollup's watch can only emit files. I wanted to serve files from memory. So I had to handle watching myself (https://rollupjs.org/guide/en/#javascript-api)

// simplified

const MEMORY = {/* [filename]: code */}

const server = await polka() // http server
server.use(serveMemory(MEMORY)) // middleware
// ...

// cssWriterRef workaround to get to css
import config, { cssWriterRef } from './rollupConfig'

// destruct regular Rollup config for Rollup API
const { output: outputOptions, ...inputOptions } = config

const handleChange = () => {
  //chokidar change callback
  const bundle = await rollup.rollup(inputOptions)
  const { output } = await bundle.generate(outputOptions)
  // update MEMORY with output
}

With such setup I can serve files from memory though only for browser (I wanted to update backend scripts dynamically with Rollup output as well, but as I use es modules, there is no way to create modules from strings yet, but it is different problem)

1) With plugin's current functionality I have to track outputs from bundle.generate and from CssWriter scoped in rollupConfig.
2) With my proposal css code (and css map) are assets of Rollup output

[JohnGurin]

Here is the updates to the code with more proper way of using plugins API.

// ...
// css(writer);

// outputOptions as the second argument allows css function
// to use the options for custom css file name
let cssFilename = css(writer, outputOptions)

// If css returns nothing then do nothing more
// Backward compatibility
if (!cssFilename)
    return

let cssName
if (cssFilename === true) {
    const {file, name} = outputOptions
    cssName = file ? {fileName : path.basename(file).replace(/\.[^/.]+$/, '') + '.css'}
            : name ? {name     : name.replace(/\.[^/.]+$/, '') + '.css'}
            : null
} else {
    cssName = {fileName: cssFilename}
}

if (outputOptions.sourcemap) {
    const isInline = outputOptions.sourcemap === 'inline'
    const map = {
                 version: 3     , sourcesContent,
                 file: null     , sources,
                 names: []      , mappings: encode(mappings),

                 toString: () => { return JSON.stringify(map) },
                 toUrl:    () => {
                    const encoding = 'base64'
                    return `data:application/json;charset=utf-8;${encoding},` +
                            Buffer.from(map.mappings).toString(encoding)
                    }
                }

    let mapURL
    if (isInline) {
        mapURL = map.toUrl()
    } else {
        const { fileName, name } = cssName || {}
        const mapName = fileName ? {fileName: fileName + '.map'}
                      : name     ? {name: name + '.map'}
                      : null
        const mapAssetId =
              this.emitFile({type: 'asset', source: map.toString(), ...mapName})
        mapURL = this.getFileName(mapAssetId)
    }
    result += `\n/*# sourceMappingURL=${mapURL} */`

    if (!isInline && !(cssName && (cssName.fileName || cssName.name))) // <------- (1)
        this.warn(`output.sourcemap was set to true. Since neither output.(file|name) ` +
                  `was provided nor exact file name was returned from svelte.css, ` +
                  `default names for css and css.map were generated`)
}
this.emitFile({type: 'asset', source: result, ...cssName})

A couple of questions arose:

1) When asset names are generated by Rollup without user options (1), file names has no extension. I did not find the way how to set .css somehow.

2) The plugin logic about css option goes like this

// Current
/*1*/ css: false|null|undefined // -> do nothing
/*2*/ css: true // -> embed styles into js
/*3*/ css: (writer) => { /* use writer */ } // -> if return nothing, do nothing

// With my modification
/*4*/ css: (_, outputOptions) => { return "exact-filename.css"  } // -> let Rollup save files
/*5*/ css: (writer) => { return true } // -> let the plugin derive names, let Rollup save files

// It would be convenient to have 
/*5*/ css: "exact-filename.css" // -> similar to /*4*/

but the following does not allow /*5*/
https://github.com/rollup/rollup-plugin-svelte/blob/f8e8fa15e5cdf87847a2b02a5471f9f4481f8edb/index.js#L144-L146
3) I'm still not quite sure what emitCss does. So I decided not to modify existing code

The whole index.js is here https://gist.github.com/JohnGurin/85160d6ca5d1a1801766d536a2b52aab

[lukeed]

The CssWriter has been removed outright as part of #147

This plugin no longer writes CSS output files at all. Instead, use emitCss: true and bring another Rollup plugin to handle the CSS pipeline.

Thank you for the ideas & feedback!