Allow plugins that transform code to return source maps, and compose them together.

[pkaminski]

Changes

Adds support for a plugin's transform callback to return a {result, map} structure, so that transformed code can be correctly mapped back to the original source. Correctly composes multiple maps together, including a base one obtained from the initial load step.

Per the discussion in #1046.

Testing

I added a simple test to check that both map pass-through and composition. I hand-checked the resulting maps (by dumping them to a human-readable format) and they looked right. We may want to set up a more complex test with "real" code and more extensive transforms, but I'm not sure how we'd verify the resulting maps.

---

This change is 

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/mq18sj86z
✅ Preview: https://snowpack-git-fork-pkaminski-transform-sourcemaps.pikapkg.vercel.app

[pkaminski]

I don't understand the test failure -- it looks as if the transform is getting the ts file before it's been transpiled, even though it gets transpiled later (otherwise the submodule.js snapshot wouldn't match). Naturally, it works fine on my machine. Any ideas? Can you check the behavior on your box? Thanks.

[drwpow]

Reading your code a bit more, now I realized you’ve placed all your functionality in our legacy plugin API. I realize it’s easy to miss with the removal of comments (I could have sworn we had comments stating “V2 API“ vs “Legacy API” for these blocks, or they got removed, or I’m thinking of another place). Basically:

if (typeof result === 'string' || Buffer.isBuffer(result)) {
  // This is our V2 API aka the way to do it going forward
} else if (result && typeof result === 'object' && (result as {result: string}).result) { 
  // This is our legacy plugin API that will be deprecated in Snowpack 3.x
  // All your code went into here 😅

Again, I apologize, as this isn’t clear at all from the code, and it should be.

Instead, I think we should do the following:

Part 1: changes to build-pipeline.ts

  if (typeof result === 'string' || Buffer.isBuffer(result)) {
    // V2 API: simple string is returned… don’t touch this
+ } else if (result && typeof result === 'object' && (result as PluginTransformResult).contents) {
+   // V2 API: maps are handled (this is where your additions should go)
  } else if (result && typeof result === 'object' && (result as {result: string}).result) { 
    // legacy API… don’t touch this either

Also, if you could add comments explaining the flow here (or even assigning these conditions as variables like isV2StringAPI, isLegacyAPI, etc.), that’d be awesome.

Part 2: changes to types/snowpack.ts

I also think we should update the types in types/snowpack.ts to reflect this new structure: for transform() plugins, either:

  /** map of extensions -> code (e.g. { ".js": "[code]", ".css": "[code]" }) */
  export type PluginLoadResult = string | SnowpackBuildMap;
+
+ export type PluginTransformResult = { contents: string; map: any }`

  export interface PluginOptimizeOptions {
    buildDirectory: string;
  }

  export interface SnowpackPlugin {
    /** name of the plugin */
    name: string;
    /** Tell Snowpack how the load() function will resolve files. */
    resolve?: {
      /** file extensions that this load function takes as input (e.g. [".jsx", ".js", …]) */
      input: string[];
      /** file extensions that this load function outputs (e.g. [".js", ".css"]) */
      output: string[];
    };
    /** load a file that matches resolve.input */
    load?(options: PluginLoadOptions): Promise<PluginLoadResult | null | undefined | void>;
    /** transform a file that matches resolve.input */
-   transform?(options: PluginTransformOptions): Promise<string | null | undefined | void>;
+   transform?(options: PluginTransformOptions): Promise<PluginTransformResult | null | undefined | void>;

Unsure about the map type… would love your feedback there.

⚠️ Note: changing this type would likely cause some type issues in the dev server, but that’s actually a good thing—I think changing the entire transform() API like you’re doing is actually pretty far-reaching and we have to make sure that it doesn’t break the dev server (I haven’t tested your PR extensively but I fear it would)

Anyway, hope this is a little bit clearer! Would love to add this functionality, and apologies again for the solution not being straightforward.

[pkaminski]

Thanks for the clarifications @drwpow! I changed things around according to your recommendations -- see some extra comments below. Also, the test is still failing on GitHub even though it runs fine on my machine and I don't understand what's wrong; your assistance in figuring it out would be appreciated. Thanks!

[pkaminski]

I shifted the string part of the type to the method definition for consistency with PluginTransformResult. (Including the string in that type makes casting a lot less useful.)

[drwpow]

Thanks, but we actually don’t want that as part of the SnowpackBuildMap. We do allow polymorphism when it comes to interfacing with plugins for flexibility and a better plugin DX. But for our own internal types we want them to be very strict. Moving that type up to SnowpackBuildMap means we have to add many more typechecks in our app than we would otherwise.

So basically we want plugin APIs to be loose and polymorphic; internal types to be strict.

[pkaminski]

I pushed the string down to load(), not up to SnowpackBuildMap. It's basically a no-op (just one other spot in the code had to be changed) and SnowpackBuildMap hasn't changed one bit. It just looked weird to have PluginLoadResult include string and PluginTransformResult not.

[drwpow]

Ah understood. Sorry for misunderstanding

[drwpow]

This is clean! Also, nice optimization with the Promise.all 👍🏻

[drwpow]

Thanks for the clarifications @drwpow! I changed things around according to your recommendations -- see some extra comments below.

🙏🏻 Awesome work! This is looking great. Thanks again for putting up with this unclear part of the codebase.

Also, the test is still failing on GitHub even though it runs fine on my machine and I don't understand what's wrong; your assistance in figuring it out would be appreciated. Thanks!

As far as the test goes, I’m actually getting the same results as the CI failure on my machine (macOS). Are you on Windows, by any chance?

Judging by the insignificant difference, I think it’s safe to update it to the expected output (I’m not totally sure what’s going on there, either, but I’m not worried about it):

- "{\"version\":3,\"sources\":[\"/home/sweet/home/snowpack/test/build/transform-sourcemap/src/submodule.js\"],\"names\":[],\"mappings\":\";AAAA\",\"sourcesContent\":[\"console.log(\\\"ts loaded\\\");\"]}"
+ "{\"version\":3,\"sources\":[\"/home/sweet/home/snowpack/test/build/transform-sourcemap/src/submodule.ts\"],\"names\":[],\"mappings\":\";AAAA\",\"sourcesContent\":[\"console.log('ts loaded');\"]}"

Maybe modifying the snapshot manually would work, at least to pass CI? But if you’re on Windows, be sure to test it on your machine, too, because we do run the entire CI suite once in Linux, and again in Windows (you’re just only seeing the Linux failure because it’s faster on GitHub to spin up, and it will always run/fail first, and cancel the in-progress Windows tests)

[pkaminski]

That's the problem though: I am on Windows, and that's where the snapshot comes from, so it looks like we have some anomalous platform-specific behavior. :( I'm concerned because the Linux snapshot appears to imply that the plugin is receiving the raw TypeScript code from submodule.ts, rather than the transpiled JS version. This doesn't make sense but I can't easily debug it unless I set up a VM... Is there already another test of a transform plugin with TypeScript sources? If not, I'm wondering if this is some kind of bug in Snowpack that my change just happened to expose.

[drwpow]

That's the problem though: I am on Windows, and that's where the snapshot comes from, so it looks like we have some anomalous platform-specific behavior. :( I'm concerned because the Linux snapshot appears to imply that the plugin is receiving the raw TypeScript code from submodule.ts, rather than the transpiled JS version. This doesn't make sense but I can't easily debug it unless I set up a VM... Is there already another test of a transform plugin with TypeScript sources? If not, I'm wondering if this is some kind of bug in Snowpack that my change just happened to expose.

Sorry for the delay; I haven’t taken the time to really dig into the core discrepancy here. We’ve dealt with similar problems before. On the one hand, I know this is frustrating. But on the other, it’s better for us to have Windows tests and make sure they’re deep tests, but the snapshots do become difficult.

I’ll try and dig into this this afternoon and come up with a better solution. Thanks for your patience on this.

[drwpow]

Dug in a little more and I think this snapshot diff may be helped by just rebasing from latest master from Snowpack? Our snapshots did change a little bit in some other areas, and I want to see if that fixes the problem here (if not fix, I expect the snapshot to be a little better, at least)

[pkaminski]

I rebased and now it's completely broken... For some reason, yarn goes and checks out dependencies separately into each test's node_modules so they no longer refer to the current snowpack I'm trying to test. I double-checked the latest contrib instructions, and tried checking out the repo from scratch and rebuilding everything but no luck. Did something change recently or did I mess things up somehow? Apologies for all the trouble. 😕

[drwpow]

@pkaminski no you’ve done an excellent job! Our snapshots have thrashed around a bit and it can be easily fixed after merging. Checking to see what @FredKSchott thinks but I’d like to just merge this as-is because you just happened to open a PR during a test change, and it’s easier to fix after merging IMO.

[FredKSchott]

LGTM! will update and confirm snapshots post-merge