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

Fix broken sourcemaps when using esbuild --public-path #515

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

richardkmiller
Copy link

When the --public-path option was added to esbuild it broke Sprockets's ability to resolve sourcemaps, even though they're still present.

Esbuild's sourcemap output with --public-path=assets:
//# sourceMappingURL=assets/application.js.map

Esbuild's sourcemap output without --public-path:
//# sourceMappingURL=application.js.map

It's desirable to keep --public-path=assets for properly resolving static assets in JavaScript files, but the leading directory in sourceMappingURL is undesirable, or at least it does not work with Sprockets.

This PR allows sourceMappingURL= to contain a leading, optional directory which is ignored as the path is resolved.

This is an alternative to #501. (Sorry, I didn't see it until after I found this solution.)

be outputted by esbuild when using its --public-path option
@capripot
Copy link

Are we still planning on merging this? Should we fix errors from tests?

fcheung added a commit to fcheung/end-to-end-typing that referenced this pull request Jun 11, 2023
@guillaumebriday-pa
Copy link

@dhh could you take a look please?

Thanks

@dhh
Copy link
Member

dhh commented Jun 18, 2023

Thanks for looking into this. Needs testing to verify behavior.

@joevandyk
Copy link

I am also running into this as well with esbuild, I have to remove the --public-path option from esbuild to get sourcemaps to work, otherwise sprockets-rails seems to remove the sourcemap comment that esbuild creates.

@joevandyk
Copy link

joevandyk commented Jan 24, 2024

would the propshaft fix at rails/propshaft#170 be applicable here?

@nicklozon
Copy link

@richardkmiller would you like me to write test cases and maybe we can get this merged?

@richardkmiller
Copy link
Author

@nicklozon Yes, that'd be great. Thanks for asking!

@nicklozon
Copy link

@richardkmiller I wrote a test and opened a PR against your repo: richardkmiller#1

I think the change makes sense and works - sprockets-rails assumes the map file will exists in the same directory as the source file so it stands to reason that any prefixed path should be removed from the sourceMappingUrl.

nicklozon and others added 2 commits March 5, 2024 11:15
…ld-public-path

Fix Missing Sourcemap with esbuild publicPath - Tests
@richardkmiller
Copy link
Author

@nicklozon Thank you for these tests! I've merged them just now.

@jroes
Copy link

jroes commented Apr 11, 2024

Would love to see this merged! Can confirm this is resolving some issues on my end.

@zacheryph
Copy link

Any ideas on when this would possibly get merged [and released] ?

thejonroberts added a commit to thejonroberts/turbo-rails-tutorial that referenced this pull request Jul 9, 2024
@qmenoret
Copy link

Clearly not a great option but I managed to hack something together with a esbuild plugin that I use only in dev mode for now:

const pathSourceMapsPlugin = {
  name: "pathSourceMaps",
  setup({ onStart, onEnd, initialOptions }) {
    initialOptions.write = false

    onEnd(async (result) => {
      const tasks = []
      for (let { path, contents, text } of result.outputFiles) {
        if (path.match(/application.js/)) {
          const newValue = text.replace(
            /\/\/# sourceMappingURL=\/assets\/application.js.map/g,
            "//# sourceMappingURL=application.js.map",
          )
          contents = new TextEncoder().encode(newValue)
        }
        tasks.push(promises.writeFile(path, contents))
      }

      await Promise.all(tasks)
    })
  },
}

It's basically hardcoding the sourcemap path, but at least I get the sourcemaps 😅

@dhh
Copy link
Member

dhh commented Aug 19, 2024

cc @rafaelfranca, do you or anyone on the team have some bandwidth to verify this? Sprockets is a bit of an unruly beast at the moment, and the criticality is high. We don't use Sprockets in active development any more (moved to Propshaft), so I'd feel better if someone who does signs off on this.

@fredrivett
Copy link

Appreciate the work everyone is doing on rails, just wanted to chime in with another vote on this one. Have spent the last couple days trying to work out why I couldn't upload sourcemaps generated with esbuild and then processed by the rails asset pipeline to datadog, and it looks like this issue is at least a part of the problem.

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.

10 participants