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

Fix broken configuration when module_type is Nil and namespace is being used. #315

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rapito
Copy link

@rapito rapito commented Sep 8, 2024

Hi!

I have a simple project that doesn't use any import/export modules and is still using sprockets.
On latest version, if I have this configuration:

JsRoutes.setup do |c|
  c.module_type = nil
  c.namespace = 'Routes'
end

It breaks like this when I load any endpoint:

image

After reviewing, I noticed that 'DTS' was being used by default causing this clash. I solved it by monkey patching it on my project, but I thought I would suggest this modification to support the intended behavior of prioritizing DTS only if the configuration_type has not been set.

@bogdan
Copy link
Collaborator

bogdan commented Sep 9, 2024

I am not sure TS definitions can be used in case when module type isn't ESM. Are you really using definitions in legacy setup? Do they work as intended helping you typecheck or autocomplete?

The problem way come from here if you are using the middleware:
https://github.com/railsware/js-routes/blob/master/lib/js_routes/middleware.rb#L41
Which generates definition no matter module type, while it should only happen for ESM (aka JsRoutes.configuration.modern?).

@rapito
Copy link
Author

rapito commented Sep 9, 2024

I am not sure TS definitions can be used in case when module type isn't ESM. Are you really using definitions in legacy setup? Do they work as intended helping you typecheck or autocomplete?

I don't mean to use ts nor definitions at all, I was just updating my project that uses the JSRoutes gem without any import/export module.

The problem way come from here if you are using the middleware: https://github.com/railsware/js-routes/blob/master/lib/js_routes/middleware.rb#L41 Which generates definition no matter module type, while it should only happen for ESM (aka JsRoutes.configuration.modern?).

That is correct, it is part of the stacktrace. Do you suggest just refactoring my PR to add an if block there so it looks something like this:

    def regenerate
      JsRoutes.generate!
      JsRoutes.definitions! if configuration.moden?
    end

@bogdan
Copy link
Collaborator

bogdan commented Sep 9, 2024

Yeah, I think this is more logical to do.... JsRoutes.definition should always use DTS module.

@rapito
Copy link
Author

rapito commented Sep 9, 2024

Yeah, I think this is more logical to do.... JsRoutes.definition should always use DTS module.

Thanks, I'll retest this tonight and resubmit.

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.

2 participants