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

[Regression] Use stable sort when prioritizing app assets #209

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

excid3
Copy link
Contributor

@excid3 excid3 commented Sep 30, 2024

This fixes a regression introduced in #206.

sort_by! doesn't preserve the original order so this loses the original order. https://ruby-doc.org/3.3.5/Array.html#method-i-sort_by-21

For duplicates returned by the block, the ordering is indeterminate, and may be unstable.

I ran into this using esbuild and had app/assets/builds as the first entry in paths. After #206, app/javascript always ended up higher priority than app/assets/builds so it was finding the wrong file.

@brenogazzola
Copy link
Collaborator

Thanks Chris! I was just talking about this one with a colleague who had this problem. Will just ask him to check and merge if it solves his problem too.

@excid3
Copy link
Contributor Author

excid3 commented Sep 30, 2024

Awesome, thanks for the quick review. It was certainly a head scratcher today as I was testing our Rails 8 migration!

@excid3
Copy link
Contributor Author

excid3 commented Sep 30, 2024

Alternative implementation I considered was to partition into two arrays and re-assign.

config.assets.paths = config.assets.paths.partition { |path| path.to_s.start_with?(Rails.root.to_s) }.flatten

@brenogazzola
Copy link
Collaborator

The partition version looks more elegant, though a sort_by! seems more intuitive. Any specific reason you chose one over the other?

@brenogazzola
Copy link
Collaborator

Just for future reference, it seems that if all elements of the array are a String, the ordering is not lost. But if you have Pathname variables in there, then the order changes.

@excid3
Copy link
Contributor Author

excid3 commented Sep 30, 2024

I figured partition would allocate a couple arrays and join them back so it is probably slower. Although the arrays probably aren't large enough to matter.

@guilhermeyo
Copy link

Thanks!!
The problem was that when reordering, having Pathname also caused the strings to be ordered, which messed up the order. Your change worked perfectly.

@excid3
Copy link
Contributor Author

excid3 commented Sep 30, 2024

Pathname objects seem pretty common because that's what you get from Rails.root.join

The ones causing issues for me are from importmap-rails: https://github.com/rails/importmap-rails/blob/main/lib/importmap/engine.rb#L46-L47

Same approach in most engines/railties I think: https://github.com/rails/mission_control-jobs/blob/main/lib/mission_control/jobs/engine.rb#L93-L94

@brenogazzola
Copy link
Collaborator

Yeah, I doubt it will make that much of a difference. But the sort_by is a bit more obvious I think.

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.

3 participants