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 development bundle with inline source maps #159

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lukaszsamson
Copy link

This PR is a PoC of a solution mentioned in #158 (comment).
While it works, it's not optimal for local development (mix dev) because it performs unnecessary optimizations and source map generation. We would require another env variable to differentiate local development from running live_dashboard as a dependancy in dev and passing --mode development in dev.exs.

Resolves #150

@josevalim
Copy link
Member

Is this building only app.dev.js with “mix dev” or both files?

@lukaszsamson
Copy link
Author

Only app.dev.js. It builds with the same settings as build.development from package.json scripts. Because the two builds have different env and mode thy need to be run as separate steps. That's why I added build script.

@josevalim
Copy link
Member

I am worried we (the maintainers) will forget to run mix build before release. I would prefer if we always built both bundles, so we don't have to worry about extra steps. Would that be possible?

@lukaszsamson
Copy link
Author

It can be done on the npm side with prebuild/prewatch scripts (and on the mix side with publish alias)

@josevalim
Copy link
Member

Could we start two watchers on the mix dev side of things?

assets/package.json Outdated Show resolved Hide resolved
@lukaszsamson
Copy link
Author

Could we start two watchers on the mix dev side of things?

If webpack build cache is race conditions free ;)

@josevalim
Copy link
Member

I see, unfortunately I am worried about having two entry points/commands to run and having them getting out of sync. That would be a deal breaker for me. :/

Another option is for us to generate the source maps file separately and then, when sending the JS code to the client, we do a String.replace(js, "example.com", "current_url"). Could that work?

@archdragon
Copy link
Contributor

Thanks for taking a look at this!

Just a quick question - why not just completely skip all optimization/minimizing when building the development bundle? Wouldn't that achieve the same effect?

@lukaszsamson
Copy link
Author

Just a quick question - why not just completely skip all optimization/minimizing when building the development bundle? Wouldn't that achieve the same effect?

Not exactly. The source is transpiled by babel and when it's bundled by webpack it looses original module structure.

@lukaszsamson
Copy link
Author

Could we start two watchers on the mix dev side of things?

If webpack build cache is race conditions free ;)

I tried that and it seems to work

js_path = Path.join(__DIR__, "../../../priv/static/js/app.js")
css_path = Path.join(__DIR__, "../../../priv/static/css/app.css")
js_bundle = if Mix.env() == "prod", do: "app.js", else: "app.dev.js"
css_bundle = if Mix.env() == "prod", do: "app.css", else: "app.dev.css"
Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, this means we will only get source map when running in development locally. If that's the case, maybe we should generate the dev bundle in a tmp directory so we don't include it in the package? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

To give a bit more context, packages always run Mix.env() == "prod". If the goal is only for local development, then I think the approach in #158 is simpler but I believe you also want source maps when used as a dep. Unfortunately though, there is no way to achieve it besides making it opt-in somehow.

@josevalim
Copy link
Member

Ping!

@lukaszsamson
Copy link
Author

@josevalim I got impression that there is no consensus on how and if to approach this. Besides even with the changes from that branch I'm not able do debug #165 as live_view and phoenix js deps come already minified and obfuscated. The better solution would be to get them unminified from npm and use js build tools in the release process.

@maennchen
Copy link
Contributor

@lukaszsamson Both Phoenix & Phoenix Live View are going to expose Source Maps starting from the next release.

One small question about this PR: Why would you only add Source Maps to the dev bundle and not to the Prod one as well?

@lukaszsamson
Copy link
Author

Both Phoenix & Phoenix Live View are going to expose Source Maps starting from the next release.

Great

One small question about this PR: Why would you only add Source Maps to the dev bundle and not to the Prod one as well?

@maennchen IIRC there were concerns about bundle size - inline source maps significantly increase download size and parsing time on index.html

@maennchen
Copy link
Contributor

maennchen commented Jun 23, 2021

@lukaszsamson Do we need inline source maps? Normal sourcemaps with a reference comment in the js should work as well, no?

Edit: never mind, we're loading it inline...

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.

Add JS sourcemaps
4 participants