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

Changes the require to the correct sprocket gem #274

Merged

Conversation

brianmcoates
Copy link
Contributor

This will fix the issue that breaks everything when trying to start your rails server. I have created an issue as well that this would close #273. I saw another fix was to add sprockets es6 to the gem file as well if that is the desired fix I can submit a pull request to update the documentation.

Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

Hello @brianmcoates 👋,

Thank you for your pull request. But I don't know Rails and the Ruby environment enough to review it.

@brianmcoates @khiet Could you me enlighten me about #273 (comment) please ? Is this pull request still required, or should I update the release notes of https://github.com/zurb/foundation-rails/releases/tag/v6.5.1.0

@khiet
Copy link

khiet commented Dec 6, 2018

Hi @ncoden,

I see three options.

⚠️ I don't know what sprockets-es6 is for.

  1. Update README to let users know gem 'sprockets-es6' has to be added to their Gemfile.

  2. Add back sprockets-es6 in the gemspec file so that it gets installed as a dependency.

  3. if sprockets-es6 is an unnecessary dependency for this gem, these gemfiles need to be cleaned up and @brianmcoates's fix is nice since Rails already comes with sprockets-rails.

My preference is 2.

@brianmcoates
Copy link
Contributor Author

@ncoden after talking to my team here we found this a6c927d#diff-44263267796d33ed2765ce27a5ba5f3c What is happening when people bundle install bundle doesn't know what the dependencies are so we need to add them back in. I have updated the pull request to do that.

@rafibomb
Copy link
Member

rafibomb commented Dec 6, 2018

I'll try to pull our Rails engineer in to check it out.

@ncoden
Copy link
Contributor

ncoden commented Dec 6, 2018

@khiet @brianmcoates Thanks a lot for the explanations!

I made a mistake, I am sorry. Now I understand better how gemspec files are used. The pull request looks good to me now, but I'm waiting for the opinoin of @rafibomb's Rails engineers to merge it.

@brianmcoates
Copy link
Contributor Author

@ncoden No worries man it happens to everyone. :) Looking forward to hearing back from your guys rails engineer.

@brianmcoates
Copy link
Contributor Author

@ncoden have you guys had a chance to talk to your rails eng?

@ncoden
Copy link
Contributor

ncoden commented Dec 12, 2018

@rafibomb

@javierjulio
Copy link

javierjulio commented Dec 18, 2018

@ncoden the right thing to do here would be to just include the following 3 gemspec dependencies:

spec.add_dependency "sass", [">= 3.3.0", "< 3.5"]
spec.add_dependency "railties", [">= 3.1.0"]
spec.add_dependency "sprockets-es6", [">= 0.9.0"]

again and release that now rather than wait. The current release is unusable because of that regression. You can at least temporarily fix that and release again to address changing dependencies.

@patricklindsay
Copy link
Contributor

Where is this Rails engineer at @rafibomb @ncoden ? :)

Foundation-sites is compatible with the latest Sass versions.
Related to foundation#265
@ncoden
Copy link
Contributor

ncoden commented Jan 23, 2019

I removed the upper limit for sass (foundation-sites support all latest Sass versions) and regenerated the lockfiles.

@ncoden ncoden merged commit 7bcafe8 into foundation:develop Jan 23, 2019
@ncoden ncoden mentioned this pull request Jan 28, 2019
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.

Updating 6.5.1.0 require 'sprockets/es6' isn't working have to change to require 'sprockets/rails'
6 participants