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

Invalid sourcemap paths for ESM and CJS bundles #192

Closed
justingrant opened this issue Nov 13, 2022 · 5 comments
Closed

Invalid sourcemap paths for ESM and CJS bundles #192

justingrant opened this issue Nov 13, 2022 · 5 comments
Assignees

Comments

@justingrant
Copy link
Contributor

justingrant commented Nov 13, 2022

The current release (0.4.2 EDIT 0.4.3) emits a sourcemap with invalid file paths, which results in console errors in IDEs like VSCode that prefer file references to inline source references in sourcemaps. I haven't kept up with recent changes in build steps, I believe this is a new problem.

Here's the first few lines of node_modules/@js-temporal/polyfill/dist/index.esm.js.map:

{"version":3,"file":"index.esm.js","sources":["../tsc-out/intrinsicclass.js","../tsc-out/slots.js","../tsc-out/calendar.js","../tsc-out/ecmascript.js","../tsc-out/regex.js","../tsc-out/intl.js","../tsc-out/instant.js","../tsc-out/plaindate.js","../tsc-out/plaindatetime.js","../tsc-out/duration.js","../tsc-out/plainmonthday.js","../tsc-out/now.js","../tsc-out/plaintime.js","../tsc-out/timezone.js","../tsc-out/plainyearmonth.js","../tsc-out/zoneddatetime.js","../tsc-out/legacydate.js","../tsc-out/index.js"]

To fix the problem, we should change ../tsc-out to ../lib. I've run into this problem before, and if my memory is correct there's either a TSC and/or a rollup setting that will allow you to set the correct directory path for the sourcemap. If that's too hard, we could just post-process the output in a build step.

@justingrant
Copy link
Contributor Author

Actually, there's two problems here:

  1. The problem described above: paths are wrong
  2. The JS files referenced in the sourcemap aren't actually included in the package.

I think we can fix both problems by simply including the tsc-out folder in the package. PR coming. There might be a cleaner solution but at least this would fix the console errors in VSCode in the meantime.

@justingrant
Copy link
Contributor Author

Note that JSBI has the same bug, so I filed GoogleChromeLabs/jsbi#97 over there.

@12wrigja 12wrigja self-assigned this Nov 14, 2022
@12wrigja
Copy link
Contributor

I think ultimately the sourcemap config here is just wrong.

We aren't actually sourcemapping back to our TS sources, but back to the ts-transpiled files. This isn't great. Instead, we should be merging the sourcemaps together so that the sourcemaps for our bundles point straight back to the TS files, without having to parse another set of embedded sourcemap urls and .map files.

I think this is a pretty simple fix, and once I've verified this I'll push a PR up to review.

@justingrant
Copy link
Contributor Author

justingrant commented Dec 3, 2022

Cool. Here's a repro you can use to validate fixes for this issue:

  • Open a new VS Code window
  • Open a new terminal inside VSCode (If you're new to VS Code, use CTRL+`) to open a new terminal pane
  • cd to some folder you can put a new repo inside of
  • npx create-react-app temporal-sourcemap --template typescript
  • cd temporal-sourcemap
  • npm i @js-temporal/polyfill
  • In the VS Code menu, use "File...Open Folder..." and choose the folder you just created above. Answer "Yes, I trust the authors of this package".
  • Now edit /src/App.tsx to add this code, and save the file:
import { Temporal } from '@js-temporal/polyfill';
const today = Temporal.Now.plainDateISO();
  • Set a breakpoint on the second line of code above. (If you're new to VS Code, you can use F9 to set a breakpoint on the line where the cursor is.)
  • Replace the static text "Learn React" (around Line 22) with this code: {today.toLocaleString()}
  • Click the debug button in the left nav bar.

image

  • Click the "Run and Debug" button in the left pane
  • When it asks to "Select debugger" choose "Web App (Chrome)"
  • VS Code will bring up a debugger config file called launch.json. Edit the port number on the url property to 3000. (default is 8080) and save the file.
  • Now we're ready to debug. Go back to the terminal pane and run npm start.
  • Click the green "play" button to start debugging.

image

  • Open up the Debug Console pane. If the bug described in this issue is still happening, you'll see this error message in the debug console:

image

  • VS Code should have stopped at the breakpoint you set above.

image

  • Now try to step into the Temporal source code by clicking the "Step Into" button at the top of the left debug pane. You may need to click it twice to actually step into Temporal. (The first click might be swallowed by the injected code that enables importing packages.)

image

  • Now you'll be in a Temporal file. If the sourcemap is working properly, you'll be in a TS file. But if it's not working properly, then you'll end up in the rollup-generated JS file, like this:

image

  • The final piece is to validate that VS Code interprets files as the correct file type. There's apparently a bug in VS Code where TS inline sourcemaps are treated as JS, which blows up the errors reported in VS Code's Problems pane when you load inline sourcemap code. If you see "JS" next to the being-debugged filename, then the bug has happened.

image

  • If you see a "TS" next to the filename, and no errors shown in the code, then things are working OK. This is what will show up when the code is on-disk in node_modules (instead of just being inline in the sourcemap file.)

image

@justingrant
Copy link
Contributor Author

Fixed by #194.

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 a pull request may close this issue.

2 participants