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.

Sign up for GitHub

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

Improve plugin-vue output #1310

Merged
merged 2 commits into from
Oct 14, 2020
Merged

Improve plugin-vue output #1310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63d574e
Improve plugin-vue output
drwpow Oct 14, 2020
e754143
Clean up empty files
drwpow Oct 14, 2020
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
4 changes: 4 additions & 0 deletions plugins/plugin-vue/plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ module.exports = function plugin(snowpackConfig) {
if (sourceMaps && js.map) output['.js'].map += JSON.stringify(js.map);
}

// clean up
if (!output['.js'].code) delete output['.js'];
if (!output['.css'].code) delete output['.css'];
Copy link
Collaborator Author

@drwpow drwpow Oct 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is a very non-controversial change here, that fixes Vue specifically.

I don’t believe other plugins had this problem—this was behavior caused by starting out with a blank code key to start, because multiple Vue things would then run in parallel to speed things up. But after everything ran we failed to check to make sure something was generated in every case (often it wasn’t)


return output;
},
};
Expand Down
12 changes: 0 additions & 12 deletions plugins/plugin-vue/test/__snapshots__/plugin-vue-ts-tsx-jsx.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,6 @@

exports[`plugin vue with jsx 1`] = `
Object {
".css": Object {
"code": "",
"map": "",
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hiding right under our noses! 🎉

Copy link
Owner

Choose a reason for hiding this comment

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

aha! Glad we have the individual test files now 👍

".js": Object {
"code": "import { Fragment } from 'vue';
import {createVNode, isVNode} from 'vue';
Expand All @@ -31,10 +27,6 @@ export default defineComponent({

exports[`plugin vue with ts 1`] = `
Object {
".css": Object {
"code": "",
"map": "",
},
".js": Object {
"code": "import {defineComponent} from \\"vue\\";
const defaultExport = defineComponent({
Expand All @@ -58,10 +50,6 @@ export default defaultExport",

exports[`plugin vue with tsx 1`] = `
Object {
".css": Object {
"code": "",
"map": "",
},
".js": Object {
"code": "import { Fragment } from 'vue';
import {createVNode, isVNode} from 'vue';
Expand Down
4 changes: 0 additions & 4 deletions plugins/plugin-vue/test/__snapshots__/plugin.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,6 @@ export default defaultExport",

exports[`plugin base only tpl 1`] = `
Object {
".css": Object {
"code": "",
"map": "",
},
".js": Object {
"code": "const defaultExport = {};
import { createVNode as _createVNode, openBlock as _openBlock, createBlock as _createBlock } from \\"vue\\"
Expand Down
35 changes: 28 additions & 7 deletions snowpack/src/build/build-pipeline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import path from 'path';
import {validatePluginLoadResult} from '../config';
import {logger} from '../logger';
import {SnowpackBuildMap, SnowpackConfig, SnowpackPlugin, PluginTransformResult} from '../types/snowpack';
import {
SnowpackBuildMap,
SnowpackConfig,
SnowpackPlugin,
PluginTransformResult,
} from '../types/snowpack';
import {getExt, readFile, replaceExt} from '../util';
import {SourceMapConsumer, SourceMapGenerator, RawSourceMap} from 'source-map';

Expand Down Expand Up @@ -84,6 +89,9 @@ async function runPipelineLoadStep(

// if source maps disabled, don’t return any
if (!sourceMaps) result[ext].map = undefined;

// clean up empty files
if (!result[ext].code) delete result[ext];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@FredKSchott This, however, is a possibly-controversial change. This asks the question: should we rely on every plugin to clean up perfectly after itself? Or should Snowpack assume an empty file shouldn’t be generated?

If the latter, then maybe this is a good change to make? I don’t see this causing any issues, unless in some cases an empty file isn’t a mistake (though I can’t think of any of those scenarios off the top of my head)

Copy link
Owner

Choose a reason for hiding this comment

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

I thought that today we treated empty string as equivalent to a file that shouldn't be built. I think i saw this issue pop up in some other work that I was doing, so maybe we need to revisit this specifically. it's also different for a plugin that creates more than one file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah so we did check that the plugin generated something, but we weren’t checking if it returned { '.js': { code: '' } }. This is the change here.

});
return result;
}
Expand All @@ -101,10 +109,14 @@ async function runPipelineLoadStep(
};
}

async function composeSourceMaps(id: string, base: string | RawSourceMap, derived: string | RawSourceMap) : Promise<string> {
async function composeSourceMaps(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ignore the other changes in this file; these are all just Prettier auto-formats.

id: string,
base: string | RawSourceMap,
derived: string | RawSourceMap,
): Promise<string> {
const [baseMap, transformedMap] = await Promise.all([
new SourceMapConsumer(base),
new SourceMapConsumer(derived)
new SourceMapConsumer(derived),
]);
try {
const generator = SourceMapGenerator.fromSourceMap(transformedMap);
Expand Down Expand Up @@ -159,22 +171,31 @@ async function runPipelineTransformStep(
// V2 API, simple string variant
output[destExt].code = result;
output[destExt].map = undefined;
} else if (result && typeof result === 'object' && (result as PluginTransformResult).contents) {
} else if (
result &&
typeof result === 'object' &&
(result as PluginTransformResult).contents
) {
// V2 API, structured result variant
output[destExt].code = (result as PluginTransformResult).contents;
const map = (result as PluginTransformResult).map;
let outputMap: string | undefined = undefined;
if (map && sourceMaps) { // if source maps disabled, don’t return any
if (map && sourceMaps) {
// if source maps disabled, don’t return any
if (output[destExt].map) {
outputMap = await composeSourceMaps(filePath, output[destExt].map!, map);
} else {
outputMap = typeof map === 'object' ? JSON.stringify(map) : map;
}
}
output[destExt].map = outputMap;
} else if (result && typeof result === 'object' && (result as unknown as {result: string}).result) {
} else if (
result &&
typeof result === 'object' &&
((result as unknown) as {result: string}).result
) {
// V1 API, deprecated
output[destExt].code = (result as unknown as {result: string}).result;
output[destExt].code = ((result as unknown) as {result: string}).result;
output[destExt].map = undefined;
}
}
Expand Down