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

Document import breaking change in @rails/ujs 7.1.0 #49768

Closed
joao-esteves opened this issue Oct 24, 2023 · 9 comments
Closed

Document import breaking change in @rails/ujs 7.1.0 #49768

joao-esteves opened this issue Oct 24, 2023 · 9 comments

Comments

@joao-esteves
Copy link

joao-esteves commented Oct 24, 2023

This change was done in 7d116c9.

Steps to reproduce

  • Add @rails/ujs on version 7.1.0 (and up) to package.json
  • On a JavaScript file write import { fileInputSelector } from "@rails/ujs";
  • Build the JavaScript (using Shakapacker, but any such system should do)

Expected behavior

Shouldn't have errors, or this breaking change should be documented in the Rails 7.1 migration guide at https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#upgrading-from-rails-7-0-to-rails-7-1.

Actual behavior

Get the error export 'fileInputSelector' (imported as 'fileInputSelector') was not found in '@rails/ujs' (possible exports: default).

System configuration

Rails version: 7.1.0 / 7.1.1

Ruby version: 3.2.2 / Any

Shakapacker version: 6.6.0 (but any other JavaScript setup should do)

@joao-esteves joao-esteves changed the title Document breaking change in @rails/ujs 7.1.0 Document import breaking change in @rails/ujs 7.1.0 Oct 24, 2023
@mguan2020
Copy link
Contributor

mguan2020 commented Oct 30, 2023

Thank you @joao-esteves for bringing this issue to our attention. Just wondering, is the error only for importing fileInputSelector, or is it for any general import from @rails/ujs?

@skipkayhil
Copy link
Member

Sorry for not getting to this sooner, let me explain what's happening

In Rails 7.0, @rails/ujs is exclusively packaged as a UMD bundle. This format is generally pretty lax about how exports work, so when used in a javascript bundle, the only exported object Rails could be imported a number of different ways:

require("@rails/ujs").start()
import * as Rails from "@rails/ujs"
import Rails from "@rails/ujs"
import { start } from "@rails/ujs"

In Rails 7.1, @rails/ujs is now packaged as both UMD and ESM. The ESM bundle enables using @rails/ujs without a javascript bundler (using importmaps for example). However, this comes with a tradeoff that the ESM format is more strict about how exports are defined. In the ESM bundle we have to be explicit about exactly what is exported and that dictates what can be imported. I chose to export default Rails because I think the most important factor was following the docs: they describes importing using a default import: https://github.com/rails/rails/tree/main/actionview/app/javascript#es2015.

Anyways, the other thing impacting this situation is that javascript bundlers seem to have decided that choosing the ESM version of a module should be opt-out rather than opt-in. So when you upgraded to Rails 7.1 your bundler is now trying to use the ESM version of the package. I think you should be able to opt out if you change @rails/ujs to @rails/ujs/app/assets/javascripts/rails-ujs.js (although I recognize that is not great).

At the end of the day, I write a lot more Ruby than Javascript, so I'd appreciate if someone more knowledgeable would be able to share any ideas to improve backwards compatibility.

@mguan2020
Copy link
Contributor

@skipkayhil Thank you very much for the detailed explanations. To check my understanding, would changing @rails/ujs to @rails/ujs/app/assets/javascripts/rails-ujs.js fix the issue with the import? Or is there something more we would have to do?

@skipkayhil
Copy link
Member

I haven't tested it so I'm not 100% sure, but that's how I expect it to work. However, that doesn't seem like a great suggestion for us to give users. If we're going to suggest they change their code, I think recommending changing to import Rails from "@rails/ujs" would be better.

@mguan2020
Copy link
Contributor

I agree; doesn't seem like recommending users to change to @rails/ujs/app/assets/javascripts/rails-ujs.js is the best idea.

Just wondering, does "import Rails from "@rails/ujs" also import the 'fileInputSelector'? Or must the users import it separately?

@skipkayhil
Copy link
Member

import Rails from "@rails/ujs"
// and then you can alias the method
const fileInputSelector = Rails.fileInputSelector
// or just reference it from the Rails object where it is used
Rails.fileInputSelector(...)

@mguan2020
Copy link
Contributor

mguan2020 commented Oct 31, 2023

Thank you for the prompt reply! I like your suggestion to modify the documentation to instruct users of the UJS to "import Rails from "@rails/ujs"". The code for that looks pretty neat.

That being said, I'm wondering if there is a way to fix this issue without having users change their existing code (which will probably involve a code change on our end).

For now, I have attached a PR #49857 with the updated documentation to explore this issue further.

@joao-esteves
Copy link
Author

import Rails from "@rails/ujs"
// and then you can alias the method
const fileInputSelector = Rails.fileInputSelector
// or just reference it from the Rails object where it is used
Rails.fileInputSelector(...)

I agree with this, and thanks for the explanation above. I didn't suggest this at first to let people more knowledgeable handle it, especially since I removed the troublesome import from my project in the first place instead of fixing it (let's just say it was practically an unused import...).

@benoittgt
Copy link
Contributor

Does it needs to be closed now because of #50535 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants