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

Remove min js file from TheAdmin #17523

Merged
merged 1 commit into from
Feb 25, 2025
Merged

Remove min js file from TheAdmin #17523

merged 1 commit into from
Feb 25, 2025

Conversation

Skrypt
Copy link
Contributor

@Skrypt Skrypt commented Feb 25, 2025

Fixes #17522

@MikeAlhayek MikeAlhayek merged commit db193e1 into main Feb 25, 2025
8 checks passed
@MikeAlhayek MikeAlhayek deleted the skrypt/#17522 branch February 25, 2025 00:56
@Piedone
Copy link
Member

Piedone commented Feb 25, 2025

Why remove instead of fixing that it wasn't created?

@Skrypt
Copy link
Contributor Author

Skrypt commented Feb 25, 2025

Please read: #17522 (comment)

@MikeAlhayek
Copy link
Member

@Piedone apparently in OC we no longer need to support two versions. We only commit the minified version to the repo. The ResourceDebugMode is no longer applicable to the OC files.

@Skrypt
Copy link
Contributor Author

Skrypt commented Feb 25, 2025

Only for assets built with Parcel. We don't need the unminified version of that file served. We need .map files which will be served or not based on the ResourceDebugMode config.

Only few of these .min files got removed from the Asset Manager PR. So, I'm not bothered at all.

@Piedone
Copy link
Member

Piedone commented Feb 25, 2025

What I remember discussing at the meeting is that eventually we won't need the non-minified versions. I wasn't aware that we can't have non-minified versions right now. That's not good, especially that if I understand correctly, map file generation is not quite there yet. Because this prevents us properly debugging JS.

@Skrypt
Copy link
Contributor Author

Skrypt commented Feb 25, 2025

Alright let me fix this all. I'll merge the other PR and then explain.
The only issue is that right now for these 5 or 6 Parcel assets we can't debug them on prod. But if you run yarn watch -n parcel-asset-name locally, it should unminify that asset while debugging it.

Just had a long discussion with @sebastienros on today's meeting on how we will add .map files.

@Piedone
Copy link
Member

Piedone commented Feb 25, 2025

Great, thank you!

@Skrypt
Copy link
Contributor Author

Skrypt commented Feb 25, 2025

So, the entire issue with Parcel is:

  • It uses the same configuration for all "parcel" actions. Meaning that right now I've set it to not output any .map files and to minify/optimize the files.
  • There is only one "source" file but it can output multiple files if you import a .scss file in your .ts file for example. So, that means it should output a .map file for both. Parcel will output these files in the same output folder and we can't split them in different folders because Parcel handles them as if they are an "app".
  • We need to clean the destination folder on every build or watch. So, the solution is to generate a .map file. I raised an issue on Parcel github repository so that they add a parameter to allow overwriting files but their advice is to clean the "dist" folder on every build. Knowing that Parcel is mainly used for building SPA applications that makes sense to never push the "dist" folder to the repository in opposite of what we do. Or if you push the "dist" folder then clean it before each build/watch which is what they advice to do.
  • It don't think there is a way with Parcel directly to rename Output asset file names because it can output multiple files. You can only set the "dest" to a specific folder where all assets will be compiled.

When we set Parcel to output a .map file it will add a line at the end of the compiled asset that gives a reference to the path where the .map file is. If we want to still use the ResourceManager to have a debug and prod asset we need to create a file that has no reference to the .map file as the prod one.

So, I will need to figure out how to trigger Parcel.run() twice to output:

file.map.js --> (debug) minified
file.map (debug) --> actual .map file
file.js (prod) --> (prod) minified but without any .map reference

Or simply harvest for files in the output folder with a separate process and remove that .map reference from it and create a new file out of it.

Or, we simply disallow hosting the .map file even though there is a reference to the .map in the file.

@Skrypt
Copy link
Contributor Author

Skrypt commented Feb 25, 2025

Or we create a new action that does exactly what we want but without using Parcel.
Just like we did with Gulp with the "typescript" action.
That's a shame but that's how the tool is built ...

I will take some time to think about it. Parcel story needs to work as much as others that allows to configure these with .config files. Quickly, I think that Vite and Webpack can produce both minified and unminified assets from a single compilation.

Also, we need to think about a solution that will work with all of these bundlers.

@Skrypt
Copy link
Contributor Author

Skrypt commented Feb 25, 2025

So to summarize I think we need both:

  • A custom build action that repro what Gulp was doing with Typescript.
  • A fix to generate .map files that will work with the ResourceManager debug and prod modes.

Though, there is always the Gulp pipeline if we're not satisfied yet of the Asset Manager. It is using Parcel, Webpack and Vite as they are built ...

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.

TheAdmin.min.js and TheAdmin-main.min.js are missing
3 participants