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

Add SourceMaps to minified JS #4276

Merged
merged 1 commit into from Jun 22, 2021

Conversation

maennchen
Copy link
Contributor

As discussed in https://groups.google.com/g/phoenix-core/c/Dk2AbXVnzeg

When running npm run build, this will:

  • Add a comment to priv/static/phoenix.js pointing to the sourcemap
  • Add a new file priv/static/phoenix.js.map

I have verified that when using webpack in your application, it will incorporate this sourcemap and therefore create better error stacktraces.

@gcauchon
Copy link

I'm not a Webpack expert, but should we also adapt thewebpack.config.js template to include a sourcemap in the production bundle too?

https://github.com/phoenixframework/phoenix/blob/master/installer/templates/phx_assets/webpack.config.js#L27

    devtool: devMode ? 'eval-cheap-module-source-map' : 'source-map',

@maennchen
Copy link
Contributor Author

@gcauchon I don't think so. The reason is that this is a decision that the user of the framework should make:

  • Open Source Applications: Shoud probably have it
  • Closed Source Applications: Most companies do not want to expose their unminified JS. If using Sentry for example a hidden-source-map can be used and uploaded instead.

@rktjmp
Copy link
Contributor

rktjmp commented Apr 2, 2021

I am a moron.

Same: #4265

@maennchen
Copy link
Contributor Author

@rktjmp I don't think so. The linked PR is for the phoenix generator. This PR only looks at the JS packaged with phoenix.

@maennchen
Copy link
Contributor Author

Rebased to current master

@maennchen
Copy link
Contributor Author

Rebased to current master

@maennchen
Copy link
Contributor Author

Amended compiled JS & Source Maps as discussed with @bcardarella

@bcardarella
Copy link
Contributor

I believe if you add to this block

template :static, [
{:text, "phx_static/app.js", :web, "priv/static/js/app.js"},
{:text, "phx_static/app.css", :web, "priv/static/css/app.css"},
{:text, "phx_static/phoenix.css", :web, "priv/static/css/phoenix.css"},
{:text, "phx_static/robots.txt", :web, "priv/static/robots.txt"},
{:text, "phx_static/phoenix.js", :web, "priv/static/js/phoenix.js"},
{:text, "phx_static/phoenix.png", :web, "priv/static/images/phoenix.png"},
{:text, "phx_static/favicon.ico", :web, "priv/static/favicon.ico"}
]
to ensure the new sourcemap file is being copied in for --no-webpack projects this PR should be ready to merge.

@chrismccord chrismccord merged commit 65c1c6d into phoenixframework:master Jun 22, 2021
@chrismccord
Copy link
Member

❤️❤️❤️🐥🔥

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

5 participants