search

jfortunato / esbuild-plugin-manifest

Esbuild plugin for building an asset manifest file.
MIT License
31 stars 5 forks source link

Add CSS entrypoints #4

Closed Richard87 closed 3 years ago

Richard87 commented 3 years ago

This looks for entrypoints for all outputs, and modifies extension.

The code needs some cleaning up, but I hope I can get some feedback before I continue :)

In our application we have 4 entrypoints, and somehow one css output is common between two of them.

{
  "../assets/FormTask/App.js": "build2/FormTask/App-HIK4HSCH.js",
  "../assets/FormTask/App.css": "build2/App-L7A7MBAS.css",
  "../assets/Search/App.js": "build2/Search/App-B3BGZYV5.js",
  "../assets/App.js": "build2/App-3DHPQ6V6.js",
  "../assets/App.css": "build2/App-L7A7MBAS.css",
  "../assets/State/index.js": "build2/State/index-VGXKL2QC.js",
  "../assets/State/index.css": "build2/State/index-4Z3VD2CO.css"
}

This was the results before my code:

{
  "../assets/FormTask/App.js": "build2/FormTask/App-HIK4HSCH.js",
  "../assets/Search/App.js": "build2/Search/App-B3BGZYV5.js",
  "../assets/App.js": "build2/App-3DHPQ6V6.js",
  "../assets/State/index.js": "build2/State/index-VGXKL2QC.js"
}

Because of the metafile's config, there might be more than one entrypoint for each file, as build2/App-L7A7MBAS.css shows, it's used by both ../assets/App.js and ../assets/FormTask/App.css. I have no idea about possible side-effects when the browser loads both the CSS files at once. but atleast it should be cached, so it shouldn't download the file more than 1 time.

Should solve #3

jfortunato commented 3 years ago

Thanks for the PR! I think this can be done more simply by just checking the metafile.outputs keys for files that should be included in the manifest. Previously that was done by checking for entrypoints only, but we could enhance that to include sibling files that we want to include.

So before we only included output files that were entrypoints:

Before

if (outputInfo.entryPoint === undefined) {
  continue;
}

But instead we could add some logic like:

After

if (!shouldIncludeInManifest(outputFilename, outputInfo)) {
  continue;
}

...

const shouldIncludeInManifest = (outputFilename, outputInfo): boolean => {
  return outputInfo.entryPoint !== undefined || outputFilename.endsWith('.css');
};

And to find the original entrypoint/input file, I believe esbuild will reference it as the last item under metafile.outputs[filename].inputs, so we should be able to get it like:

// gets the original entrypoint (or sibling file entrypoint)
let input = Object.keys(outputInfo.inputs).reverse()[0];
Richard87 commented 3 years ago

Thanks for the feedback! I tottally agree about limiting the output that wil be refrenced

(especially since esbuild will only bundle js and css anyway!)

Personnaly i dont trust that the last item will always be the target entrypoint, but I think it would be a good idea to start the search there in case esbuild change in the future!

I will try to clean up the proof of concept tomorrow, it got a little messy while making it work 😅

Richard87 commented 3 years ago

I added a import './global.css' in example.js to the tests, but the extensionless feature breaks...

jfortunato commented 3 years ago

That's because esbuild will name the file the same as the entry js file, and will cause a naming conflict if extensions aren't used. For example:

// filename: example.js
import './global.css';

console.log('example');

Esbuild will output 2 files, example.js and example.css. When the extensions are dropped, they will both just be example.

But anyway I don't think we want the manifest file to be using example.css as a key at all. I think we would want the key to be the name of the original file, which is global.css in this case. So with the previous example the manifest.json should be:

{
  "example.js": "example.js",
  "global.css": "example.css",
}
Richard87 commented 3 years ago

I don't agree on using the asset css name. Because the rest of my framework (symfony in this case) shouldn't know about the css it's entrypoints are using, except that it is using css (I don't see any way to avoid it!), I want to keep the main name...

But it's your call :) 

{
  "example.js": "example.js",
  "global.css": "example.css",
}

For example in my app, I get this manifest:

{
  "../assets/FormTask/App.js": "build2/FormTask/App-HIK4HSCH.js",
  "../assets/FormTask/App.css": "build2/FormTask/App-KAYXNLJB.css",
  "../assets/Search/App.js": "build2/Search/App-B3BGZYV5.js",
  "../assets/App.js": "build2/App-VRKFLDW4.js",
  "../assets/App.css": "build2/App-LENKXH5T.css",
  "../assets/State/index.js": "build2/State/index-VGXKL2QC.js",
  "../assets/State/index.css": "build2/State/index-YCUL2GUI.css"
}

and in my "micro-spa-page", I can add these lines:

<link href="{{ asset('../assets/State/index.css') }}" rel="stylesheet" >
<script src='{{ asset('../assets/State/index.js') }}'></script>

Since the stylesheet is predictable and follow my entrypoints, it's easier to reason about (in my opinion)...

esbuild.build({
    entryPoints: [
        '../assets/FormTask/App.js',
        '../assets/Search/App.js',
        '../assets/App.js',
        '../assets/State/index.js',
    ],
//....
})
Richard87 commented 3 years ago

About the shouldIncludeInManifest(), maybe we can just check the bundle property (without it esbuild don't do anything)?

jfortunato commented 3 years ago

The common usecase for using the manifest file is to have known inputs map to unknown outputs. In your example:

<link href="{{ asset('../assets/State/index.css') }}" rel="stylesheet" >

I think you are relying on esbuilds convention of using the same name as the js file as the known input. If esbuild changed the convention, or if certain options are used (like code splitting), it would break that expectation. If you just ensure that your source css file is actually named index.css, it will make no difference anyway, you could use the same exact line.

Also the changes as-is will not work with code splitting. Consider this example:

// test/input/example-with-css/index-with-splitting.js
import('./example');
import('./example2');

console.log('index with splitting');

And the options:

esbuild.build({
    entryPoints: ['test/input/example-with-css/index-with-splitting.js'],
    outdir: 'test/output',
    plugins: [manifestPlugin()],
    bundle: true,
    splitting: true,
    format: 'esm',
});

The manifest.json will not include a css entry at all.

jfortunato commented 3 years ago

I'm going to merge this in, but I want to test a few more things and I think I will probably make the change to use the original name in the manifest before making a new release.

Richard87 commented 3 years ago

You are right, I assume code splitting would require some front-end runtime to download the chunks, both css and js.

But if I need to know the css output name from before, I might as well just add it to my entrypoints. The feature I had in mind was for when js-files imported css, then it should keep the entrypoints name, but with a css extension.

In my use-case, we have a "legacy" symfony project, where I'm building new features in different react-apps, so I have a few known entrypoints (the js apps), where some of them also use css-files.

Richard87 commented 3 years ago

The common usecase for using the manifest file is to have known inputs map to unknown outputs. In your example:

<link href="{{ asset('../assets/State/index.css') }}" rel="stylesheet" >

I think you are relying on esbuilds convention of using the same name as the js file as the known input. If esbuild changed the convention, or if certain options are used (like code splitting), it would break that expectation. If you just ensure that your source css file is actually named index.css, it will make no difference anyway, you could use the same exact line.

Also the changes as-is will not work with code splitting. Consider this example:

// test/input/example-with-css/index-with-splitting.js
import('./example');
import('./example2');

console.log('index with splitting');

And the options:

esbuild.build({
    entryPoints: ['test/input/example-with-css/index-with-splitting.js'],
    outdir: 'test/output',
    plugins: [manifestPlugin()],
    bundle: true,
    splitting: true,
    format: 'esm',
});

The manifest.json will not include a css entry at all.

The code splitting feature is interresting, I have no idea what kind of side-effects that would have in a simple implementation, or if the output format changes things. I'm very new to esbuild, just wanted to try it out in our application for the speed, and this was the only roadblock (with mininal changes to my current code atleast!)