search

ezolenko / rollup-plugin-typescript2

Rollup plugin for typescript with compiler errors.
MIT License
822 stars 71 forks source link

Directory in sourcemap is incorrect, impacting `vite serve` (but not Rollup) #407

Open luxaritas opened 2 years ago

luxaritas commented 2 years ago

Troubleshooting

  1. Does tsc have the same output? If so, please explain why this is incorrect behavior

    No, due to this plugin changing outDir

  2. Does your Rollup plugin order match this plugin's compatibility? If not, please elaborate

    As far as I know - I have it configured as a pre plugin with vite

  3. Can you create a minimal example that reproduces this behavior? Preferably, use this environment for your reproduction

    https://stackblitz.com/edit/vitejs-vite-rjjhlb?file=vite.config.js

What happens and why it is incorrect

When this plugin emits sourcemaps, the source map URL is incorrect, as the plugin configures typescript with a temporary output directory, and the sourcemap is generated relative to that. The sources property is modified in the .d.ts.map, but not similarly adjusted in the .js.map.

This works fine with Rollup itself, as when it merges sourcemaps it (from what I can tell) winds up only looking at the "root" sourcemap in the sourcemap chain. However, when using vite, it's serve mode does not use rollup and instead uses @ampproject/remapping to combine sourcemaps, which relies on the sources URL being properly formed.

If you look at the script served by vite, you get: 

sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJtYXBwaW5ncyI6IkFBQUEsT0FBTztBQUNQLE9BQU8sb0JBQW9CO0FBQzNCLFNBQVMsb0JBQW9CO0FBRTdCLFNBQVMsY0FBOEIsTUFBTSxFQUFHLFlBQVk7QUFBQTtBQUFBO0FBQUE7QUFBQTtBQUFBO0FBQUEsa0JBTTFDO0FBQUE7QUFBQTtBQUFBO0FBQUE7QUFBQTtBQUFBO0FBQUE7QUFBQTtBQUFBO0FBQUE7QUFZbEIsYUFBYSxTQUFTLGNBQWlDLFVBQVUsQ0FBRSIsIm5hbWVzIjpbXSwic291cmNlcyI6WyIuLi8uLi8uLi8uLi9zcmMvbWFpbi50cyJdLCJmaWxlIjoiL2hvbWUvcHJvamVjdHMvdml0ZWpzLXZpdGUtcmpqaGxiL3NyYy9tYWluLnRzIiwic291cmNlc0NvbnRlbnQiOltudWxsXX0=

which decodes to:

{"version":3,"mappings":"AAAA,OAAO;AACP,OAAO,oBAAoB;AAC3B,SAAS,oBAAoB;AAE7B,SAAS,cAA8B,MAAM,EAAG,YAAY;AAAA;AAAA;AAAA;AAAA;AAAA;AAAA,kBAM1C;AAAA;AAAA;AAAA;AAAA;AAAA;AAAA;AAAA;AAAA;AAAA;AAAA;AAYlB,aAAa,SAAS,cAAiC,UAAU,CAAE","names":[],"sources":["../../../../src/main.ts"],"file":"/home/projects/vitejs-vite-rjjhlb/src/main.ts","sourcesContent":[null]}

The sources property should read ["../src/main.ts"]. Inserting log statements into this plugin confirms that this is what is being output from the plugin.

While technically this is a difference in behavior between vite and rollup and could arguably be a vite bug, my feeling was that this plugin should still expose accurate source URIs, especially since this is already handled for the .d.ts.map

Environment

Versions

System:
    OS: Linux 5.0 undefined
    CPU: (4) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 16.14.2 - /usr/local/bin/node
    Yarn: 1.22.10 - /bin/yarn
    npm: 7.17.0 - /bin/npm
  npmPackages:
    rollup-plugin-typescript2: ^0.32.1 => 0.32.1 
    typescript: ^4.6.4 => 4.7.4 
    vite: ^3.0.2 => 3.0.4

vite.config.js

: ```js import { defineConfig } from 'vite'; import typescriptPlugin from 'rollup-plugin-typescript2'; export default defineConfig({ plugins: [ { ...typescriptPlugin({}), enforce: 'pre', }, ], }); ```
luxaritas commented 2 years ago

This may be the actual issue raised in https://github.com/ezolenko/rollup-plugin-typescript2/issues/294

luxaritas commented 2 years ago

I realized to get this working for now, I can pass:

tsconfigOverride: {
    compilerOptions: {
        sourceRoot: '../src'
    }
}

From what I understand, sourceRoot (or changes to the entries in sources itself) would always need to be resolved from the location of the output... which isn't known during build hooks. Theoretically, you could specify an absolute path to the source files, but this would only work locally (e.g. fails when running on stackblitz). Does this mean Vite's sourcemap merging approach would have to be changed to account for this, or is there some other way of reliably being able to determine how to resolve the source location?

agilgur5 commented 2 years ago

Thanks for filing a detailed issue and adding a workaround @luxaritas!

This may be the actual issue raised in #294

Good to know! It was closed by the OP and they never responded to requests for more info so 🤷

Mapping logic

as the plugin configures typescript with a temporary output directory, and the sourcemap is generated relative to that. The sources property is modified in the .d.ts.map, but not similarly adjusted in the .js.map.

Coincidentally, I'm intimately familiar with this logic given that I just wrote tests for .d.ts.map remapping in #403 and was the one who filed #204 and fixed it in #221 . When I was writing the tests, I was curious if .js.map files might be prone to this issue as well -- good timing!

While technically this is a difference in behavior between vite and rollup and could arguably be a vite bug, my feeling was that this plugin should still expose accurate source URIs, especially since this is already handled for the .d.ts.map

So #221 is more a workaround for .d.ts.map as they were simply unusable without that. .js.map, as you say, is useable within Rollup, so there isn't a need to fix anything there. If Vite isn't correctly replicating Rollup behavior, then I would argue that is a Vite bug. Note that this is a Rollup plugin that has existed well before Vite did, and not a Vite plugin.

Also note that the temporary / "placeholder" directory was a required workaround for #83 / https://github.com/Microsoft/TypeScript/issues/24715 (which are 4+ years old now). So this is workarounds on workarounds now 😕

From what I understand, sourceRoot (or changes to the entries in sources itself) would always need to be resolved from the location of the output... which isn't known during build hooks.

Indeed, that's something I ran into in https://github.com/ezolenko/rollup-plugin-typescript2/pull/221#issuecomment-701119370, it's only known during output generation hooks.

If we could use sourceRoot instead that would definitely be preferred, as the current logic of deserializing and reserializing JSON sourcemaps to change a single field is pretty inefficient.

Theoretically, you could specify an absolute path to the source files, but this would only work locally (e.g. fails when running on stackblitz).

Using an absolute path would be pretty easy, though I'm not totally sure why it doesn't work on Stackblitz. It does still have a filesystem (which is needed for rpt2 and many Node libraries), so the error it's giving (Sourcemap for "/home/projects/vitejs-vite-rjjhlb/src/counter.ts" points to missing source file) seems more that it can't serve the file perhaps?

Does this mean Vite's sourcemap merging approach would have to be changed to account for this, or is there some other way of reliably being able to determine how to resolve the source location?

We could change sources in generateBundle or another output generation hook as you can change the existing bundle object (though the preferred and efficient way is to know ahead of time during transform), but vite serve doesn't run output generation hooks what-so-ever, so that doesn't really leave another option (and the bundle object probably has the correct sourcemap too since that's at the end after Rollup combines all the mappings).

We would basically have to change a tsconfig setting to account for this during a build hook, so either sourceRoot or outDir etc.

A different solution to the "placeholder" dir is to require users to set outDir and then check during output generation that it actually matches the Rollup output dir. vite serve would never run this check though 😕 Honestly, that's also not very different from the workaround you showed here of putting a sourceRoot in tsconfigOverride. That check is something I was thinking of with respect to rpt2's useTsconfigDeclarationDir option, which is similar, but works on declarationDir and does not check the Rollup output dir (it bypasses Rollup altogether to output declarations). That alternative also has the caveat of potentially still hitting TS errors if there's something already in the Rollup output dir (versus the placeholder dir is non-existent).

So an absolute path may be the only viable option from rpt2's side. I'd have to dig in a bit more and remember my trials in #221 to see if there are any alternatives. Vite or @ampproject/remapping solving this may indeed be a better option.

More notes on Vite compatibility

  • As far as I know - I have it configured as a pre plugin with vite

Ah I forgot Vite had the enforce: 'pre' option. I was telling folks to disable esbuild for TS files, but this could be simpler.

That being said, Vite doesn't actually describe in its docs what are its "core plugins" or "build plugins", so I had to do a bit of digging into the source code to see if this was compatible. pre is here, near the top of the list, with esbuild below it. That part is correct, but resolve is actually below as well, which can be problematic, particularly when importing node_modules. Vite is using a custom resolver as well, so I'm also not sure how it otherwise plays along. It's also just a huge number of plugins in that list, using Vite is far from a minimal example as such.

So I don't think I can validate that this is entirely compatible, and the resolve ordering seems problematic too.

I'm not sure if Vite has a better option to fully customize plugin ordering. Order typically matters to Rollup plugins, Babel plugins, etc.

It might be better to configure esbuild to not run on TS files during build with exclude: /\.tsx?$, and then run rpt2 as a normal plugin.

Integration issues

To be clear, Vite maintainers have never engaged with rpt2, so we've just had a few ad-hoc issues from users that aren't even sure what plugins Vite uses, or, often, haven't realized that esbuild compiles TS either. It's not an ideal scenario, especially when Vite is much newer than rpt2, has much more maintainers and funding, and abstracts out a lot for their users, meaning that their users are not well-equipped to file issues with respect to integrations, especially minimal ones. This is different from the normal Rollup ecosystem, where users have to add all plugins on their own and so the level of abstraction is not so different. For reference, I used to maintain TSDX, which was an abstraction around Rollup, but I filed issues here in rpt2 myself, as a maintainer on behalf of users (and eventually became a maintainer here). #204 is actually an example of one of those issues.

luxaritas commented 2 years ago

Thanks for the detailed response. I'm well aware that this plugin is designed for Rollup and note Vite, hence my note that I had considered filing an issue against Vite instead. And I was fully hoping to PR a fix until I realized it may well be intractable. :) For clarity the main reason I'm using rpt2 is that 1) I want support for typescript features like experimentalDecorators and 2) @rollup/plugin-typescript was having some bizarre issues in watch mode where the builds it output were out of date from the source... it may be worth revisiting, but to some degree that's neither here nor there. I would be interested if you could point me to what bugs pop up from esbuild and resolve being configured how they are, but well noted.

Using an absolute path would be pretty easy, though I'm not totally sure why it doesn't work on Stackblitz. It does still have a filesystem (which is needed for rpt2 and many Node libraries), so the error it's giving (Sourcemap for "/home/projects/vitejs-vite-rjjhlb/src/counter.ts" points to missing source file) seems more that it can't serve the file perhaps?

This (afaik) comes down to how the browser resolves it. If it's an absolute path, it looks for it on your local filesystem (ie, the filesystem the browser is running on, not the filesystem of the server) - but its not there, it should be requesting it from the server. Though again it seems weird that Vite isn't normalizing this to a relative path...

At any rate, this behavior (+ workaround) is at least now recorded here, and I'll start poking the vite side of things.

agilgur5 commented 2 years ago

I'm well aware that this plugin is designed for Rollup and note Vite, hence my note that I had considered filing an issue against Vite instead. And I was fully hoping to PR a fix until I realized it may well be intractable.

Ah, my apologies if my tone was too strict on that one! To be more explicit, this is the best issue report we've gotten from a Vite user by a pretty wide margin (and in general a good issue report that's clearly done its reading 🙂 ). What I intended by that statement was that a Vite-exclusive compatibility issue should probably default to a Vite issue, as opposed to the other way around, which I've seen a bit of in various Rollup plugins' issues. I feel the Vite team & community should try to reel that in due to the factors I mentioned above. Comments have the dual audience of current OP as well as future readers, so it's hard to strike a balance with that regard; that wasn't specifically aimed at you in that sense, apologies.

1) I want support for typescript features like experimentalDecorators

a bit different than the usual use-case I see from Vite users (type-checking and declaration generation), but a great example of a use-case 👍

2) @rollup/plugin-typescript was having some bizarre issues in watch mode where the builds it output were out of date from the source... it may be worth revisiting, but to some degree that's neither here nor there.

@rollup/plugin-typescript has diverged quite a bit from the original implementation nowadays and uses some newer TS APIs. With regard to watch mode, IIRC it uses the TS watcher and tries to synchronize that with the Rollup watcher. Even my Rollup watch mode tests here (#386) had some problems alongside Jest's watch mode, so I can see a lot of unexpected edge cases resulting from that 🤷

I would be interested if you could point me to what bugs pop up from esbuild and resolve being configured how they are, but well noted.

With enforce: 'pre', you shouldn't hit into any esbuild bugs as rpt2 will run before it. If you run rpt2 after esbuild (as a normal plugin or post), that can cause a lot of issues, because it seems that rpt2 is running on the now compiled, untyped JS as opposed to TS. https://github.com/ezolenko/rollup-plugin-typescript2/issues/305#issuecomment-1200261316 seems to be one such example (that I recently re-visited).

With regard to resolve, you can see some older issues around ordering of rollup-plugin-node-resolve in #87, #67, etc. If rpt2 is before resolve, then rpt2 will try to resolve the path using TS (and your current tsconfig moduleResolution), whereas one likely wants resolve to go first. Per Vite's source code that I linked above, enforce: 'pre' places rpt2 before resolve, so that could cause some issues.

This (afaik) comes down to how the browser resolves it. If it's an absolute path, it looks for it on your local filesystem (ie, the filesystem the browser is running on, not the filesystem of the server) - but its not there, it should be requesting it from the server. Though again it seems weird that Vite isn't normalizing this to a relative path...

Reading the sourceRoot docs, it sounds like that might be intended behavior? As it seems designed for URLs, but can be used in this way as well. But I'm not a sourcemap / debugger expert (yet?) to know how all of those fields are intended to work 🤷

So an absolute path may be the only viable option from rpt2's side. I'd have to dig in a bit more and remember my trials in #221 to see if there are any alternatives.

A bit of digging later, it seems like manipulating outDir or sourceRoot are still the only options as far as I can tell 😕

The other Rollup option I was thinking of as a workaround was sourcemap: "inline" or its other sourcemap configurations (below in the link), but those are actually all output options as well 😐
Vite's sourcemap option seems to only configure for build too...

At any rate, this behavior (+ workaround) is at least now recorded here, and I'll start poking the vite side of things.

👍

luxaritas commented 2 years ago

Ah, my apologies if my tone was too strict on that one!

No worries at all! Appreciate the additional context, just wanted to make sure we're on the same page which it seems we are. Thanks again for taking the time to look into this.

luxaritas commented 2 years ago

Quick update: I've realized that using ../src is incorrectly resolving for submodules, as it's relative to the sourcemap location, so eg if we have a file at dist/sub/file.ts, ../src would be dist/, not src. I've also learned that sourceRoot with an absolute path does work. If you go into your browser's source view, it doesn't automatically collapse the two files like it does if you resolve the two from the same location. Go figure, since they're not actually "in the same place". :laughing: Vite will resolve http://<host>/some/local/path to /some/local/path and serve it up for you. If you console.log and click on the link to where it was logged from, it does indeed give you the source file!

luxaritas commented 2 years ago

So, that being said: I thinks this means this can be fixed in this plugin by setting sourceRoot to the absolute path of whatever the resolved rootDir is (accounting for the fact if you don't set rootDir, it's the common path of all source files, not necessarily .). I can't guarantee that this file serving behavior works the same for other dev servers though.

luxaritas commented 2 years ago

One more note: Vite (edit: or rather rollup, which has always ignored the path issues) will actually resolve the sourcemaps to local paths and not absolute paths for you when doing a build, so this also works there too - no worries about production sourcemaps pointing to a local path on a build machine.