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

Upgrade rake assets:precompile to update js-routes files in Rakefile did not work for me #305

Closed
tae8838 opened this issue Mar 24, 2023 · 13 comments

Comments

@tae8838
Copy link

tae8838 commented Mar 24, 2023

namespace :assets do
  task :precompile => "js:routes:typescript"
end

wasn't working for me. I had to do Rake::Task["assets:precompile"].enhance(["js:routes:typescript"]) instead.

@bogdan
Copy link
Collaborator

bogdan commented Mar 24, 2023

We can change it like that, but I would want to know exactly why it doesn't work for you and does work for everyone else.

Can you debug it?

@tae8838
Copy link
Author

tae8838 commented Mar 28, 2023

Sorry, I still ran into the same problem with Rake::Task["assets:precompile"].enhance(["js:routes:typescript"])

The problem is when I don't have routes.js and I'm generating it for the first time, rails assets:precompile gives me this error:

Error: Build failed with 3 errors:
ERROR: Could not resolve "../../routes"
ERROR: Could not resolve "../../routes"
ERROR: Could not resolve "../../routes"

After I run rails js:routes:typescript for the first time and the routes.js has been generated, it works.

@bogdan
Copy link
Collaborator

bogdan commented Apr 3, 2023

I believe that for some reason the files that use import {} from '../../routes' are being checked for correct imports before js:routes:typescript task being executed. I am not sure whey does it happen and can not reproduce it with a new rails application.

Can you try a new rails application if it has the same problem for you and let me know what kind of custom configuration you are using? That might be linters, typescript tools or something else.

@abinoam
Copy link

abinoam commented May 23, 2023

Hi @tae8838

From where does this message come from? (webpacker?, rails assets:precompile?, when deploying?)

I'm using esbuild, and in my case I have to guarantee that bin/rails js:routes:typescript is run before yarn build as my javascript files "import" the routes. If I run in sequence, everything work ok because esbuild will keep watching for changes in generated route files. If yarn build is run before bin/rails js:routes:typescript it will give me an import error. (I think that's what @bogdan is telling with "the files... are being checked for correct imports before...".

I'm hooking into your issue because I'm trying to configure everything to work well in development, test and production.
As per documentation we should config.middleware.use(JsRoutes::Middleware) in config/environments/development.rb. So I'm guessing how it should work in test environment.

I guess that locally it will have a high chance of an up to date routes.js file as we will be probably be using our development environment. But I don't know how it would work in a CI.

I'll be probably relying on configuring everything externally without the middleware.

@abinoam
Copy link

abinoam commented May 23, 2023

I've just read that js-bundling hooks into assets:precompile in production just like js-routes recommended way.
And in testing it hooks into test:prepare.

@abinoam
Copy link

abinoam commented May 23, 2023

It seems jsbundling also doesn't try to guarantee any kind of running sequence. It just hooks normally.
See: https://github.com/rails/jsbundling-rails/blob/main/lib/tasks/jsbundling/build.rake

@abinoam
Copy link

abinoam commented May 23, 2023

HI @tae8838,

I think I was able to reproduce your scenario.
Can you give me the output of bin/rails --prereqs | grep -A 5 assets:precompile
In my case:

rails assets:precompile
    environment
    javascript:build
    css:build
    js:routes:typescript

So, if I:

  • rm app/javascript/routes.d.ts app/javascript/routes.js
  • bin/rails assets:precompile

It gives me:

$ node config/esbuild.config.mjs
✘ [ERROR] Could not resolve "./routes"

    app/javascript/dashboard.js:6:24:
      6 │ import * as Routes from './routes';
        ╵                         ~~~~~~~~~~

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
rails aborted!
jsbundling-rails: Command build failed, ensure yarn is installed and `yarn build` runs without errors
/home/abinoam/src/ruby/gomedical/bin/rails:9:in `<top (required)>'
/home/abinoam/src/ruby/gomedical/bin/spring:15:in `<top (required)>'
Tasks: TOP => assets:precompile => javascript:build
(See full trace by running task with --trace)

But if I:

  • bin/rails js:routes:typescript
  • bin/rails assets:precompile

It works ok.

That's because esbuild throws an error when one of my js files tries to import routes.js and it is not there yet (because js:routes:typescript will be running after javascript:build)

Dear @bogdan,

I think I can try to open a PR for it so we can discuss the code.

@abinoam
Copy link

abinoam commented May 23, 2023

@tae8838

Could you test this? It worked for me.

# lib/tasks/js_routes.rake

# If javascript:build defined, hook into it and everything is fine.
# It will run before it in assets:precompile and in test:prepare
if Rake::Task.task_defined?("javascript:build")
  Rake::Task["javascript:build"].enhance(["js:routes:typescript"])
else
  Rake::Task["assets:precompile"].enhance(["js:routes:typescript"]) if Rake::Task.task_defined?("assets:precompile")
  Rake::Task["test:prepare"].enhance(["js:routes:typescript"]) if Rake::Task.task_defined?("test:prepare")
  Rake::Task["spec:prepare"].enhance(["js:routes:typescript"]) if Rake::Task.task_defined?("spec:prepare")
  Rake::Task["db:test:prepare"].enhance(["js:routes:typescript"]) if Rake::Task.task_defined?("db:test:prepare")
end

@bogdan
Copy link
Collaborator

bogdan commented May 23, 2023

@abinoam great job on investigating this. Can you clarify where test:prepare and spec:prepare tasks are coming from?

I am also not sure why would we need to generate js-routes on db:test:prepare. What kind of tests can require that?

Last but not least: I would also check if assets:precompile depends on javascript:build beside the last one just being defined. That may help to ensure stronger evidence that javascript:build should be extended.

@abinoam
Copy link

abinoam commented May 23, 2023

I'm at the cell phone now. Later I can crafter a more long answer. But, the short answer is javascript:build is defined by Rails official jsbundling-rails gem

And, at its source code I can see how it hooks.

https://github.com/rails/jsbundling-rails/blob/main/lib/tasks/jsbundling/build.rake

Although it may exist, Im not aware of another javascript:build task injected by a different gem.

I need more time to be sure if this would be the "optimal" and more compatible way of doing that. Let's keep discussing more about it until we figure the vest way of doing that so we can update the Readme (as I think we won't need to change js-routes source code)

@abinoam
Copy link

abinoam commented May 25, 2023

Can you clarify where test:prepare and spec:prepare tasks are coming from?

In my setup test:prepare comes from Rails itself and from Cucumber. spec:prepare comes from rspec-rails.

$ bin/rails -W test:prepare
rails db:test:prepare                /home/abinoam/.rvm/gems/ruby-3.2.2/gems/activerecord-7.0.4.3/lib/active_record/railties/databases.rake:573:in `block (2 levels) in <main>'
rails test:prepare                   /home/abinoam/.rvm/gems/ruby-3.2.2/gems/railties-7.0.4.3/lib/rails/test_unit/testing.rake:19:in `block in <main>'
rails test:prepare                   /home/abinoam/src/ruby/gomedical/lib/tasks/cucumber.rake:63:in `<main>'
$ bin/rails -W spec:prepare
rails spec:prepare                   /home/abinoam/.rvm/gems/ruby-3.2.2/gems/rspec-rails-6.0.2/lib/rspec/rails/tasks/rspec.rake:22:in `block in <main>'

I am also not sure why would we need to generate js-routes on db:test:prepare. What kind of tests can require that?

You're right about it. But I just included it because jsbundling-rails does that. See at: https://github.com/rails/jsbundling-rails/blob/main/lib/tasks/jsbundling/build.rake#L19

Last but not least: I would also check if assets:precompile depends on javascript:build beside the last one just being defined. That may help to ensure stronger evidence that javascript:build should be extended.

This will make the code more resilient. I found the way to do it (Rake::Task#prerequisites)

Looking at jsbundling-rails I'm not sure that the way it does is the best way to do it.
In my case I use both Cubumber and RSpec. When javascript:build binds to test:prepare it doesn't bind do spec:prepare.

For js-routes I think we should bind it to both. This may guarantee that there will always be an up to date routes file.

The js_routes.rake file I'm testing is now like this. What do you think about it @bogdan ?

And... perhaps we should fix only the documentation of js-routes (the Readme part where it suggests the js_routes.rake).
With such an example file it would be easier for anyone to adapt to its own setup.

# lib/tasks/js_routes.rake

# frozen_string_literal: true

# See discussion at https://github.com/railsware/js-routes/issues/305

# If javascript:build is defined, hook into it and everything is fine.
# If not, hook into assets:precompile.

if Rake::Task["assets:precompile"].prerequisites.include?("javascript:build")
  Rake::Task["javascript:build"].enhance(["js:routes:typescript"])
else
  Rake::Task["assets:precompile"].enhance(["js:routes:typescript"])
end

if Rake::Task.task_defined?("test:prepare") && Rake::Task["test:prepare"].prerequisites.exclude?("javascript:build")
  Rake::Task["test:prepare"].enhance(["js:routes:typescript"])
end

if Rake::Task.task_defined?("spec:prepare") && Rake::Task["spec:prepare"].prerequisites.exclude?("javascript:build")
  Rake::Task["spec:prepare"].enhance(["js:routes:typescript"])
end

if Rake::Task.task_defined?("db:test:prepare") && Rake::Task["db:test:prepare"].prerequisites.exclude?("javascript:build")
  Rake::Task["db:test:prepare"].enhance(["js:routes:typescript"])
end

@bogdan
Copy link
Collaborator

bogdan commented May 25, 2023

I think it is better to keep user level control over assets building process and tasks. Making it a blackbox may cause problems in custom assets building processes. For example, legacy apps with sprockets or when the harddrive is readonly for some reason.

spec:prepare depends on test:prepare so at least we don't need that: https://github.com/rspec/rspec-rails/blob/main/lib/rspec/rails/tasks/rspec.rake#L26

On the db:test:prepare : I don't see a reason we should extend that task, especially because it makes running non-frontend tests slower.

bogdan added a commit that referenced this issue Jul 5, 2023
Whenever jsbundling-rails is used, it is better to extend
javascript:build instead of assets:precompile. See #305.
@bogdan
Copy link
Collaborator

bogdan commented Jul 5, 2023

Released 2.2.6 with updated readme and generator. Thanks for help, @abinoam

@bogdan bogdan closed this as completed Jul 5, 2023
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

No branches or pull requests

3 participants