-
Notifications
You must be signed in to change notification settings - Fork 151
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
Generate source maps and configure digest bypass #21
Conversation
I had to remove the the |
Why do we need to keep the chunk count low? Seems like an optimization someone should just consider whether they need or not if it's a problem. Don't think we need that as a default. |
(Just that we're trying to keep an absolute minimal config, just enough to make things work. Then someone can tweak as they see fit from there.) |
I added that because the current file has a plugin to keep chunk count at 1. I thought it was a priority. I’ll remove an retest to ensure everything is working, then update. |
Ah, no, I only kept it at one chunk so we wouldn't have to deal with the issue with digested chunks not working 👍 |
7075ecd
to
be7f6c2
Compare
Removed and tested. It's ok. We don't need to try to keep chunk count low. |
Sweet. Will merge once we've pushed out a new sprockets release. Just waiting for a review from @rafaelfranca on that. |
Are you able to get this to work as expected in development as well with Or is the idea to not split chunks in development? |
Hu... I kept testing it only in production mode with Now if only I could figure out what is routing |
An easier option would be to allow chunking in dev, but disable hashing. The |
@brenogazzola Do you have a public repository or gist with a testing app for this issue? I'm interested in looking at some approaches to solving this and other use cases, and if there isn't already a public repository perhaps we could sync up on creating one. |
Another approach I tried out was to reverse the order of the name and the hash |
@rmacklin Here you go: lazy-loading-jsbundling. I've created this with:
Then I switched If you go to Dynamic compiling (development):
Precompiled (production):
From what I learned about sprockets when submitting my PR, the precompiled version is relying on the manifest file which knows exactly where clipboard.js is, while the dynamic compiling is relying on something else and that is what needs fixing. |
Ok, the one to blame here is the It is intentionally stripping away the fingerprint in the requested file path. I went through the history and got this: From the comments it seems the original developers were not sure how valuable this behavior was. But since it was intentionally added, I don't think we can modify this without someone with considerably more experience in sprockets weighting in. |
Like I mentioned before, a possible fix is disabling hashing in development const environment = process.env.NODE_ENV;
module.exports = {
output: {
chunkFilename: environment === 'production' ? '[name]-[contenthash].js' : '[name].js',
}
} This however means that to get the correct behavior in production it's necessary to have |
What about adding a configuration option for that and have |
I tested commenting out the Disabling hashing in dev is enough to fix the problem without risking unintentionally breaking something in existing apps. |
@brenogazzola I've made an attempt to solve this in Sprockets: rails/sprockets#717 This approach works fine in my environment, using the same esbuild configuration for development and production. |
Ah, nice. I like that solution. Let's hope the maintainer are happy with it too so we can get all this merged. |
@theodorton can you test this on my sample app? I'm not sure if I'm doing something wrong, but now that your PR is merged its breaking on Make sure you do What I'm seeing is that for For Location: | 22: #
| 23: # The String Asset URI is returned or nil if no results are found.
| 24: def resolve(path, load_paths: config[:paths], accept: nil, pipeline: nil, base_path: nil)
| 25: paths = load_paths
| 26:
| => 27: if valid_asset_uri?(path)
| 28: uri, deps = resolve_asset_uri(path)
| 29: elsif absolute_path?(path)
| 30: filename, type, deps = resolve_absolute_path(paths, path, accept)
| 31: elsif relative_path?(path) At this point the |
I'm actually wondering how sprockets, when we are not messing with digests, knows to search the |
Don't |
Oooops. You are right. Now that I'm deleting the files instead of the folder everything works. 🚀 |
be7f6c2
to
bc54192
Compare
Adjusted the chunk name to be |
bc54192
to
13b2a6f
Compare
Added sourcemaps. |
Hello. I am using your latest sprockets pull request and i am trying to try esbuild source maps on development mode. require('esbuild').build({
entryPoints: ['app/javascript/application.js'],
bundle: true,
outdir: 'app/assets/builds',
sourcemap: true,
// splitting: true,
// format: 'esm',
// chunkNames: '[name]-[hash]',
entryNames: '[name]-[hash].digested',
minify: !(process.env.NODE_ENV === 'development'),
watch: watch
}).catch(() => process.exit(1)) i keep getting asset not found exception from this line I have cleared tmp/cache and i have been really careful not to delete the 'app/assets/builds/' folder. Did you do anything else in order to resolve it? |
Yeah, esbuild is not compatible with our current solution. By setting the So your application file is not |
@thanosbellos we still need to have hexadecimal digests in esbuild as well, because the regex checking for a digest in As a workaround, since you're not using splitting, you can just omit the hash from esbuild and rely on propshaft producing the digest for you. Or do you have a specific use case in mind for having the hash in the entry file name? |
Thanks for your answers to both of you. @theodorton tbh i am more or less experimenting with the new tools and esbuild. i don't have something specific in mind. More or less i would love to have splitting and source maps. Based on your comments and some search, this is what i have got working so far and i think is similar to the webpack proposed solution. Keep in mind that i do use the sprockets version with @brenogazzola latest pull request that does permit esbuild like hash names. require('esbuild').build({
entryPoints: ['app/javascript/application.js', 'app/javascript/admin.js'],
chunkNames: '[name]-[hash].digested',
bundle: true,
outdir: 'app/assets/builds',
splitting: true,
format: 'esm',
watch: watch
}).catch(() => process.exit(1)) Unfortunately i can't really use propshaft right now. Although i am looking forward to it. I have not migrated to rails 7 alpha yet. |
Right now if you need sourcemaps, the only working options are: If you are already in jsbundling I'd suggest staying in webpack for now while we try to solve the esbuild problem. |
I think if we changed the digest bypass to only search the |
We can't have a solution where "javascript-something.js" is not digested in production. So need to figure something else out with that. Then other option, especially for esbuild, is just to output ES6 directly. Then you don't even need a source map. |
Ended up making sprockets capable of dealing with source map comments created by transpilers directly via 3.4.0, so just leaned on that for #50. This work is still valuable re: chunks, but let's treat that as a separate PR. Also, have to replicate the source map handling I've added to sprockets-rails in Propshaft. Do you want to take a look at that @brenogazzola? |
Depends on rails/sprockets#714
Updates Webpack's config to make it hash any chunk it generates. This causes Sprockets to skip fingerprinting the chunks, and therefore not breaking Webpack features that rely on knowing the chunk's exact name, such as lazy loading:
@dhh I've tested this one by creating a new project from the current Rails
main
, and forcing bundler to use the sprockets PR we were discussing, then applying this configuration, and running on production mode. Worked great.