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

Remove coffeescript from this library #188

Merged
merged 7 commits into from
Sep 13, 2023
Merged

Remove coffeescript from this library #188

merged 7 commits into from
Sep 13, 2023

Conversation

rafaelfranca
Copy link
Member

Don't force the coffee-rails library as dependencies of applications.

Don't force the coffee-rails library as dependencies of applications.
@rafaelfranca
Copy link
Member Author

Tests are green and diff of resulting JavaScript doesn't show any big differences.

Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to review up to turbograft/initializers.js, but I can continue later.

lib/assets/javascripts/turbograft/click.js Outdated Show resolved Hide resolved
lib/assets/javascripts/turbograft/click.js Outdated Show resolved Hide resolved
lib/assets/javascripts/turbograft/click.js Outdated Show resolved Hide resolved
lib/assets/javascripts/turbograft/component_url.js Outdated Show resolved Hide resolved
lib/assets/javascripts/turbograft/csrf_token.js Outdated Show resolved Hide resolved
lib/assets/javascripts/turbograft.js Show resolved Hide resolved
lib/assets/javascripts/turbograft/component_url.js Outdated Show resolved Hide resolved
test/teaspoon_env.rb Show resolved Hide resolved
Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any show-stopping issues, so I'll approve. It might be worthwhile to introduce eslint for better formatting in some places as a followup: https://github.com/Shopify/web-configs/tree/19c8d94e5a7703a2f021b746790591adf34dbe3a/packages/eslint-plugin

@rafaelfranca
Copy link
Member Author

This repository will probably not evolve further. All code in our apps that use it should at this point be deleted. All code that is kept, should just use Turbo. There is no maintainer for this library, no CI. I'm just doing this migration because I want to remove CoffeeScript from the applications we are using this gem.

I don't see need to add eslint, or anything to improve this code further given that.

if (link.constructor === Link) {
return link;
}
Link.__super__.constructor.call(this, link.href, link);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I figured out that this line needs to be changed to make all the class syntax to work:

Suggested change
Link.__super__.constructor.call(this, link.href, link);
super(link.href, link);

Otherwise, JS complains that a constructor was not called with new.

@paracycle paracycle force-pushed the rm-decaffeinate branch 5 times, most recently from 2599cc4 to 553bcfb Compare September 12, 2023 22:37
paracycle and others added 2 commits September 13, 2023 01:50
We are not using GitHub actions.
@rafaelfranca rafaelfranca merged commit b807f33 into master Sep 13, 2023
4 checks passed
@rafaelfranca rafaelfranca deleted the rm-decaffeinate branch September 13, 2023 22:56
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems September 14, 2023 19:15 Inactive
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