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.

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

Create combined sourcemaps that point directly to TS source files #194

Merged
merged 3 commits into from
Dec 7, 2022

Conversation

12wrigja
Copy link
Contributor

Previously, the sourcemap generation done by rollup wasn't reading the inline sourcemaps from the files in tsc-out and using those to create new sourcemaps, so the sourcemaps in our bundles were pointing to generated code.

Now that the TS sources are embedded into the sourcemaps, we can also stop shipping the lib directory.

Fixes (hopefully) #192.

I'm still investigating how this would work for JSBI - applying the same technique there yields invalid sourcemaps.

sources.

Now that the TS sources are embedded into the sourcemaps, we can also
stop shipping the lib directory.
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

LGTM but someone else who knows about NPM packaging should take a look as well... @justingrant?

@12wrigja 12wrigja marked this pull request as ready for review November 16, 2022 17:04
@12wrigja
Copy link
Contributor Author

@justingrant , can you specifically look at this change as if we'd released it within the context of the problem you encountered previously?

I've manually verified the sourcemaps should be "useful" using sourcemap-validator on them, but whether they are actually useful when debugging is something I've found much harder to try and validate.

Steps to mimic publishing this:

  • checkout this PR

  • npm run build to build the new package contents (outputs should be unchanged from v0.4.3, but the maps are different)

  • npm pack to pack that up into a .tar.gz file that, when publishing, is uploaded to NPM.

  • in a project that uses this polyfill, install the archive file to overwrite Temporal for that project: npm install <path to the .tar.gz from before>.

    • This will change the package-lock.json and possibly also the package.json, so I'd suggest making a commit of the project and doing this with a clean working directory so you can easily reset the repo later
    • don't forget to rm -rf /node_modules/@js-temporal afterwards to forcibly re-install the package from NPM with npm install . I ran into some weird conditions where I seemingly couldn't revert back to published package versions, but this fixed it
    • build project / repro problem.

    I'd sometimes use npm link for this, but I don't want to accidentally rely on the folder structure of the project source (which won't be published anymore with this commit as the same contents is already in the sourcemaps themselves), and the flow above mostly mimics what happens when people install the package anyways.

@justingrant
Copy link
Contributor

@12wrigja A problem with inline-only sourcemaps (without shipping source files) is that VSCode prefers using files. For example, if you want to set a breakpoint in a library, but the app isn't running, if there's no original source available, then there's nowhere to set that breakpoint because the "source" doesn't exist until runtime. And if you're trying to debug a startup issue then it's particularly problematic.

That said, I'll try this PR in the debugger and report back what I find.

@12wrigja
Copy link
Contributor Author

@justingrant friendly ping - any updates? Alternatively, if you can describe how your environment is setup that would be best so I can dive into it in detail when I have time.

@justingrant
Copy link
Contributor

@12wrigja sorry have been out on vacation and then am wrapping up a job this week. Will have more time later this week and next week to dig into this.

@justingrant
Copy link
Contributor

Hi @12wrigja - I tested this. Overall looks good! The only change I'd recommend is to include the lib folder in the files config in package.json.

Doing this will avoid a bug in VS Code where inline sourcemap code is always treated as JS, even when the original source is TS. See the end of #192 (comment) for more details about this problem.

Having original source files on-disk also makes it easier to set breakpoints in library code when the app isn't running, because the files will still be in node_modules and therefore available for breakpoints.

package.json Outdated
@@ -75,7 +75,6 @@
"index.d.ts",
"index.d.cts",
"dist",
"lib",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend leaving lib here to make things easier for VSCode debugging. See #194 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's the VS Code bug that's triggered by removing lib from the package: microsoft/vscode#168013

Copy link
Contributor

@justingrant justingrant left a comment

Choose a reason for hiding this comment

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

Works well. Thanks! Also should make a similar change over in the JSBI repo.

@justingrant
Copy link
Contributor

FYI @12wrigja - as part of testing this I found some errors showing up in later versions of TypeScript, so as soon as this PR is merged I'll create a PR to upgrade dependencies in this repo and fix the new errors reported by TS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants