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.

Sign up for GitHub

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

Jump to bottom

Rails 6 assets compatibility #808

Merged
merged 5 commits into from
Nov 7, 2019
Merged

Rails 6 assets compatibility #808

merged 5 commits into from
Nov 7, 2019

Conversation

tanema
Copy link
Contributor

@tanema tanema commented Oct 29, 2019 •
edited
Loading

Design of this PR:

Rails 5

  • JS assets stay where they are with assets manifests still existing so that old javascript_include_tags will still work
  • javascript_include_tags will still work with an added rails6 check

Rails 6

  • Javascript files are copied to app/javascript/shopify_app
  • An index.js file is created to include what we need in the application.js pack with required('shopify_app')

TODO still

  • ShopifyApp::Engine precompile
  • Check for webpacker
  • Add an attribute to force Sprockets

@tanema tanema force-pushed the rails_6_generator branch from 618a08d to 07ec4f4 Compare October 29, 2019 20:23
@tanema tanema mentioned this pull request Oct 30, 2019
Closed
@tanema tanema changed the title Rails 6 compatibility Rails 6 assets compatibility Oct 30, 2019
@ziaulrehman40
Copy link

I used these changes manually(and partially) and it worked, please release this compatibility as soon as possible. Thanks.

@tylerball tylerball marked this pull request as ready for review November 7, 2019 19:50
@tylerball tylerball requested a review from a team as a code owner November 7, 2019 19:50
@tylerball tylerball merged commit 87836e4 into master Nov 7, 2019
@tylerball tylerball deleted the rails_6_generator branch November 7, 2019 20:34
smathieu
smathieu reviewed Dec 5, 2019
View reviewed changes
shopify_app.gemspec
@@ -14,6 +14,7 @@ Gem::Specification.new do |s|
s.add_runtime_dependency('rails', '> 5.2.1')
s.add_runtime_dependency('shopify_api', '~> 8.0')
s.add_runtime_dependency('omniauth-shopify-oauth2', '~> 2.2.0')
s.add_runtime_dependency('dotenv-rails', '~> 2.7.5')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this added a runtime dependency? This forces all apps to add dotenv, which not really needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smathieu smathieu smathieu left review comments

@tylerball tylerball tylerball approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tanema @ziaulrehman40 @tylerball @smathieu