-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Asset Manager Sourcemaps #17533
base: main
Are you sure you want to change the base?
Asset Manager Sourcemaps #17533
Conversation
Adds sourcemaps to assets. Adds a .prod.js that can be used to not share the .map file on prod.
This is no longer a |
Opened ticket here and linking for tracking: swc-project/swc#10108 |
For info, my last commit is sent from a compilation done in Windows 11. |
This reverts commit fe0e927.
@@ -78,37 +78,30 @@ glob(config.source).then((files) => { | |||
if (fileInfo.ext === ".js") { | |||
let reader = await fs.readFile(file, "utf8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the issue I think is that fs on windows will read this file with \r\n
in it and on linux it will read it with \n
Then when it creates the .map file it generates different mappings hash because that file length is longer on Windows making it fall on more lines making the hashes to differ.
When reading the comments on that issue, we just need to specify the line endings for the related files (e.g. JS):
|
Me and @sebastienros tried that 2 weeks ago and it did not work. We tried a lot of different combinations of that git parameter without success. The differentiation is that this is a text file with \r\n actually written litterally in them. It's not the "newline" char itself. When you read my post here I'm typing \r\n and it doesn't make it fall to a newline because it is litteral. Also, whether you are on Windows or Linux when you run I made it work now. |
Yes, I know it is about the escaped \r\n in the map files. But your current solution is also fine for me. |
Yes, the issue would not be fixed because we generate these files on the CI and locally when developping. Of course it can alter that file when cloning but that's part of the issue. I've had to read this to understand how a sourcemap works: The thing is that on Windows when we read that file with the Asset Manager. It has \r\n chars which makes that file being different. When the sourcemap mappings are getting created, it does a hash that basically has in it position of lines and chars position on that line which makes the hash differ from Windows or Linux when you compile it with SWC. Is it an issue in SWC? ... not 100%. SWC generates from what it gets. But I'd expect maybe at least an option to make sure that it make it consistent in compiling assets and .map files. We do not have this issue with Parcel because they probably figured it out already. And I believe Parcel uses SWC. |
That's what I think we can avoid. We can just use \n on Windows as well (or \r\n on Linux respectively, I don't care) and therefore avoid the whole problem. This is what the git attribute does and the map files should be the same because the source files will never have \r\n but only \n. |
Just note that if you change the gitattributes, the local working copy must be updated, e.g. with
Because of that your current solution is much simpler for the OC devs. |
I remember that we tried that also locally on my PC. I may retry it to see what happens now. |
Fix imports in Asset Manager
@@ -1,5 +1,7 @@ | |||
# Gulp Pipeline | |||
|
|||
Will be deprecated. Use new [Assets Manager](../assets-manager/README.md) instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as discussed
.SetDependencies("bootstrap", "admin-main", "theme-manager", "jQuery", "Sortable") | ||
.SetUrl("~/TheAdmin/js/theadmin/TheAdmin.prod.js", "~/TheAdmin/js/theadmin/TheAdmin.js") | ||
.SetVersion("1.0.0"); | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc for file.prod.js
Fix yarn clean to remove Scripts and Styles folders since we removed the necessity of the dest param in assets.json file.
Arrghhh!!! Did a yarn clean and found that we were not copying everything from the Assets.json to the wwwroot folders in the OrchardCore.Resources module. Now, I need to clean this mess before getting back to this PR. 🫨 |
Adds sourcemaps output to assets compilation/minification.
Adds a .prod.js that can be used to not share the .map file on prod.
Makes the "Resource Debug Mode" configuration usable with Parcel output assets.