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

Handle deferred route drawing in URL helpers DSL compiler #1899

Merged
merged 2 commits into from
May 14, 2024

Conversation

bravehager
Copy link
Contributor

@bravehager bravehager commented May 13, 2024

Motivation

The URL helpers DSL compiler appears broken after rails/rails@e97db3b introduced deferred route drawing.

This leads to all helper methods being stripped from GeneratedUrlHelpersModule and GeneratedPathHelpersModule.

Implementation

Ensure routes are loaded before generating GeneratedUrlHelpersModule and GeneratedPathHelpersModule.

Tests

I was able to manually test this out in one of our Rails applications at @justworkshr.

Open to any ideas on how to unit test this behavior—it seems like the existing tests pass despite the change in Rails itself.

@bravehager bravehager requested a review from a team as a code owner May 13, 2024 17:23
@andyw8 andyw8 added the bugfix label May 13, 2024
@andyw8
Copy link
Contributor

andyw8 commented May 13, 2024

Thanks! Wondering if we need a test for this... it seems fairly low risk. @egiurleo?

@@ -102,6 +102,9 @@ class << self
def gather_constants
return [] unless defined?(Rails.application) && Rails.application

routes_reloader = Rails.application.routes_reloader

This comment was marked as resolved.

@bravehager bravehager requested a review from andyw8 May 13, 2024 17:58
@egiurleo
Copy link
Contributor

The UrlHelpersSpec tests don't even seem to fail when running them with Rails main (RAILS_VERSION=main bundle install && bin/test spec/tapioca/dsl/compilers/url_helpers_spec.rb). I've been trying to write a integration test for this but I think it would require pretty significant changes to the DSL integration spec, which doesn't seem worth it.

I'm testing against Shopify core to make sure this doesn't break anything.

@egiurleo egiurleo merged commit 871d641 into Shopify:main May 14, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants