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

Add a test helper #341

Closed
wants to merge 3 commits into from
Closed

Add a test helper #341

wants to merge 3 commits into from

Conversation

gauravtiwari
Copy link
Member

@gauravtiwari gauravtiwari commented May 7, 2017

@dhh As we discussed the other day this addresses compiling assets in tests based on @javan's idea. Also removed the rake task enhancements because it's compiling twice and could also lead to confusion, this way it's more clear. What do you think?

We will need to merge the reload PR #292 first because that will make sure that manifest change has been picked up whenever it changes.

@gauravtiwari gauravtiwari requested a review from dhh May 7, 2017 17:15
@gauravtiwari
Copy link
Member Author

screen shot 2017-05-07 at 18 15 20

Add a delete method to manifest

Remove delete for now

Remove exist? for now and document in readme

Fix wording

Fix sentence

Add a checksum so it doesn't compile unless changed

Add namespace

Make rubocop happy
end

def compile_webpack_assets
Rails.cache.fetch(["webpacker", "manifest", checksum]) do
Copy link
Contributor

Choose a reason for hiding this comment

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

This cache key will only be invalidated when your files change, right? What if, for example, you bump the version of a dependency in package.json? I think it'd be best to avoid caching unless we can use the same digests provided by webpack.

Copy link
Member Author

@gauravtiwari gauravtiwari May 9, 2017

Choose a reason for hiding this comment

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

@javan Yepp that's right. Good point 👍 Perhaps we should watch package.json and yarn.lock for changes too? The compile task slows down the tests so it's best to cache because we won't be able to get new digests from webpack until it's compiled (which is same thing as re-compiling) unless I am missing something??

Copy link
Member Author

Choose a reason for hiding this comment

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

Added yarn.lock and package.json to watch list

@javan
Copy link
Contributor

javan commented May 10, 2017

Closing in favor of #360

@javan javan closed this May 10, 2017
@gauravtiwari gauravtiwari deleted the add-test-helper branch May 10, 2017 16:51
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.

2 participants