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

Cache test pack compilation #539

Merged
merged 3 commits into from
Jun 30, 2017
Merged

Cache test pack compilation #539

merged 3 commits into from
Jun 30, 2017

Conversation

gauravtiwari
Copy link
Member

@gauravtiwari gauravtiwari commented Jun 26, 2017

Fixes: #468
Discussion Ref: #360


Make test compilation cacheable and configurable so the lazy compilation only triggers if files are changed under tracked paths. Following paths are watched by default -

  ["app/javascript/**/*", "yarn.lock", "package.json"]

To add more paths:

# config/initializers/webpacker.rb or config/application.rb
Webpacker::Compiler.watched_paths << 'bower_components'

cc // @richseviora @stevehanson @raldred @kernow

@gauravtiwari gauravtiwari requested a review from javan June 26, 2017 21:26
@richseviora
Copy link

@gauravtiwari This is brilliant, thank you! :)

@stevehanson
Copy link

@gauravtiwari looks great! ❤️

@raldred
Copy link

raldred commented Jun 26, 2017

Excellent work, thanks @gauravtiwari much appreciated.

@@ -4,7 +4,8 @@ default: &default
source_path: app/javascript
source_entry_path: packs
public_output_path: packs
compile_missing_packs: true

compile_missing_packs: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Why flip this?

Copy link
Member Author

@gauravtiwari gauravtiwari Jun 27, 2017

Choose a reason for hiding this comment

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

I wasn't quite clear on the usage and it seemed that the main purpose of this is to compile assets during lookup when running tests. For rest of the cases, either dev server or watcher can be used. By default we turn if off and can be toggled per env. WDYT?

@@ -35,7 +36,13 @@ test:

public_output_path: packs-test

compile_missing_packs: true

compiler_cache_path: tmp/.webpacker-checksum
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't need to be configurable, IMO. How about tmp/webpacker/checksum? Then any future temp files can share that dir.

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense 👍

compiler_tracked_paths:
- app/javascript/**/*
- yarn.lock
- package.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps make these the defaults internally and add an API for watching additional paths. Like Spring does: https://github.com/rails/spring#watching-files-and-directories

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be nice. Will push something later 👍

compile_task.invoke
compile_task.reenable
end

private
def source_checksum
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if checksum is the best term since it's not computing a hash. Maybe something more explicit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah me too. May be source_timestamp?

@gauravtiwari
Copy link
Member Author

@javan Pushed some updates. Please review when you get a chance

@@ -4,7 +4,7 @@ default: &default
source_path: app/javascript
source_entry_path: packs
public_output_path: packs
compile_missing_packs: true
compile_missing_packs: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename this to just compile? The "missing" part no longer applies.

Copy link
Member Author

Choose a reason for hiding this comment

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

@javan Sure, that would make more sense or may be test_compiler_enabled: for clarity- I was looking at this for 5 minutes and finally had to refer to PR to understand what this does 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this is a test env feature. It's quite useful in dev mode too.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true - much like webpack watcher (but slow) 👍

@@ -3,12 +3,48 @@
module Webpacker::Compiler
extend self

# Default paths watched for changes
DEFAULT_WATCHED_PATHS = ["app/javascript/**/*", "yarn.lock", "package.json"].freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Watch config/webpacker.yml and config/webpacker/**/* too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes - may be we can just watch - config/webpack/**/*

# Additional paths that test compiler needs to watch
# Webpacker::Compiler.watched_paths << 'bower_components'
mattr_accessor :watched_paths
@@watched_paths = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Can set default values with a block: mattr_accessor(:watched_paths) { [] }

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Update changelog

Fix test

Update webpacker

Fix wordings

Change method name for clarity

Update readme

Remove webpack-dev-server bit for now

Update toc

Add tests for new configuration

Move configuration to compiler class and make it configurable programmatically

Fix readme

Only run compiler in test env

Add compiler test

Change method name

Remove Rails.root

Use stub

Rename to compile and update syntax

Update changelog

Revert escaping
@gauravtiwari gauravtiwari merged commit b8e3873 into rails:master Jun 30, 2017
@gauravtiwari gauravtiwari deleted the cache-test-compilation branch June 30, 2017 21:21
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.

5 participants