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

Make Propshaft compatible with esbuild's --public-path option #150

Conversation

gjtorikian
Copy link

esbuild has a config option, --public-path, which is necessary to use for integration with jsbundling-rails; this comment explains why it's desirable to have.

Unfortunately, when combining this option with source map generation, Propshaft (and Sprockets, too) is unable to read the sourcemap location, because it doesn't recognize a prefixed directory path. This PR, which was adapted wholly from
rails/sprockets-rails#515, fixes the issue, and enables esbuild users who need source map generation to integrate with Propshaft.

@brenogazzola
Copy link
Collaborator

brenogazzola commented Sep 1, 2023

I'm not sure I understand the change. The .+ in the regex should already allow it to match anything, including the forward slash you added. Also, in the test file already includes, at line 23, a test for a nested source map which is not only nested in assets, but also in a second directory.

@gjtorikian gjtorikian force-pushed the fix-missing-sourcemap-with-esbuild-public-path branch from ed772e8 to dd32663 Compare September 2, 2023 22:26
These  may be outputted by esbuild when using its `--public-path` option
@gjtorikian
Copy link
Author

The issue isn't in the regex per se, but the source_mapping_url function below.

I broke my commit into two, so that you could see that sourcemap output which esbuild emits (//# sourceMappingURL=assets/application.js.map) is not parsed. That's because the provided directory, assets, can't be found by assembly.load_path. So we need to explicitly ignore any preceding directories ((?:.*\/)?) in order for this comparison to work.

@gjtorikian
Copy link
Author

@brenogazzola 👋 Would it be helpful if I created a fresh Rails repo to demonstrate the problem this is fixing?

@brenogazzola
Copy link
Collaborator

It would yes. Or at least reproduction steps. This way it's easier for me to compare things and figure out if there's anything else that needs to change 🙏

@gjtorikian
Copy link
Author

Okay! I created a repo here: https://github.com/gjtorikian/propshaft-esbuild-error

Open up package.json and you'll notice that we are transpiling TS into JS, and the command to generate the final JS is:

esbuild app/javascript/*.* --chunk-names=[name]-[hash].digested --splitting --format=esm --bundle --minify --sourcemap --outdir=app/assets/builds --public-path=/assets/

Run bundle install; npm install, and start the server with bundle exec foreman start.

If you pop open the dev console, you'll see this:

Screenshot 2023-09-15 at 17 06 42

That's incorrect, because the source maps aren't being included.

In the Gemfile, introduce my fork and run bundle install:

gem "propshaft"
# gem "propshaft", git: "https://github.com/gjtorikian/propshaft", ref: "fix-missing-sourcemap-with-esbuild-public-path"

Run the server again, open the dev console, and you should see:

Screenshot 2023-09-15 at 17 07 21

The source map is correctly included.

I started from a fresh rails new, and this is the one commit which introduces the stack of propshaft, jsbundling-rails, esbuild, and TypeScript: gjtorikian/propshaft-esbuild-error@a0343d3

Let me know if you need more info!

@gjtorikian
Copy link
Author

You know, now I'm even wondering...do I even need jsbundling-rails here? The propshaft README offers:

On the other hand, if you're already bundling JavaScript and CSS through a Node-based setup, then Propshaft is going to slot in easily. Since you don't need another tool to bundle or transpile. Just to digest and serve.

I always thought jsbundling-rails was just a Ruby way to call an npm task, but is it possibly interfering here?

@gjtorikian
Copy link
Author

Bumping this. #133 introduced a conflict that doesn’t affect this original issue.

@gjtorikian
Copy link
Author

@brenogazzola gentle bump. I fixed the previous merge conflict. Even a confirmation that I am doing something wrong with my propshaft setup would be useful.

@brenogazzola
Copy link
Collaborator

Ops, sorry about the delay. I'm still trying to figure out if this is a config error or a bug in propshaft.

Debugging your project, the problem seems to be the final option you added: --public-path=/assets/.

If you remove it, it will work. Propshaft indexes the source map as application.js.map and this options is causing it to look for it in assets/application.js.map.

I thought that was it, but after precompiling its broken again. So I need a bit more time to see if I can find a config that makes both work.

@gjtorikian
Copy link
Author

Debugging your project, the problem seems to be the final option you added: --public-path=/assets/.

Thank you for the reply, but in the OP I mentioned:

esbuild has a config option, --public-path, which is necessary to use for integration with jsbundling-rails; this comment explains why it's desirable to have.

I'm really trying to figure out what the right thing to do is here.

@tagCincy
Copy link
Contributor

tagCincy commented Dec 11, 2023

For anyone encountering this issue with esbuild and propshaft, you can monkey patch the compile with the regex in this PR:

# config/initializers/propshaft_sourcemappingurl_patch.rb

class Propshaft::Compiler::SourceMappingUrls
  PREFIXED_SOURCE_MAPPING_PATTERN = %r{(//|/\*)# sourceMappingURL=(?:.*/)?(.*\.map)(\s*?\*/)?\s*?\Z}
  def compile(logical_path, input)
    input.gsub(PREFIXED_SOURCE_MAPPING_PATTERN) do
      source_mapping_url(
        asset_path(::Regexp.last_match(2), logical_path),
        ::Regexp.last_match(1),
        ::Regexp.last_match(3)
      )
    end
  end
end

@brenogazzola
Copy link
Collaborator

Merged #170 as that seemed the more correct solution since it removes the url_prefix instead of everything.

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.

None yet

3 participants