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

Lazy compile in test env #360

Merged
merged 5 commits into from May 10, 2017
Merged

Lazy compile in test env #360

merged 5 commits into from May 10, 2017

Conversation

@javan
Copy link
Member

@javan javan commented May 10, 2017

  • Adds new Webpacker.compile API (that invokes the webpacker:compile rake task)
  • Lazily compiles in test env to avoid Webpacker::FileLoader::* exceptions
  • Eliminates the need enhance Rails test tasks

Differences from #341

  • Doesn't require any changes to an application's test classes
  • Doesn't attempt to recompile when files change
@gauravtiwari
Copy link
Member

@gauravtiwari gauravtiwari commented May 10, 2017

@javan This looks really good 👍 After we discussed the other day I realised that re-compilation isn't necessary if we restart the dev-server/watcher programmatically whenever a pack is added/deleted (at the moment it doesn't). That would be good enough because it will ensure that manifest gets updated, which will ensure that tests will run just fine. I think we will still need #292 to make sure manifest change gets picked up in test environment though.

@dhh
Copy link
Member

@dhh dhh commented May 10, 2017

Lovely! ❤️

/play makeitso

@javan javan merged commit 549988b into rails:master May 10, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@javan javan deleted the javan:lazy-compile-in-test-env branch May 10, 2017
@javan javan mentioned this pull request May 10, 2017
@gauravtiwari gauravtiwari mentioned this pull request May 10, 2017
@jackjennings
Copy link

@jackjennings jackjennings commented May 18, 2017

Would love to get this in a release—any plans for cutting a new version of the gem with this change soon?

@dhh
Copy link
Member

@dhh dhh commented May 18, 2017

@gauravtiwari
Copy link
Member

@gauravtiwari gauravtiwari commented May 19, 2017

Shall we wait until Monday? I was thinking to finish up README PR for this release too.

@dhh
Copy link
Member

@dhh dhh commented May 19, 2017

@pmk1c
Copy link

@pmk1c pmk1c commented May 24, 2017

Is it correct, that I still have to run webpack-dev-server during tests with this change? Seems to be the case. The assets get compiled during test, but the asset url Webpacker generates is "http://localhost:8080/packs/application.js"

EDIT: Switching to current master and regenerating configuration files fixes this. Thanks!

@zacharyw
Copy link

@zacharyw zacharyw commented May 28, 2017

Is it best practice to check these compiled test assets into version control, or no?

@javan
Copy link
Member Author

@javan javan commented May 29, 2017

@zacharyw, no. I'd .gitignore them.

@Jeremy-Walton
Copy link

@Jeremy-Walton Jeremy-Walton commented Jun 2, 2017

@javan I updated my webpacker gem like @pmk1c and now it will generate the public/packs-test when I run my tests. However, once I have that folder, any changes I make to my packs are not being recompiled when I run the tests and I have to delete my public/packs-test folder to get it to regenerate it. Is there something I am doing wrong here? Shouldn't it recompile for the tests when there have been changes made?

@zacharyw
Copy link

@zacharyw zacharyw commented Jun 3, 2017

@Jeremy-Walton I'm seeing the same thing. As a workaround I'm back to manually compiling assets before spec runs:

config.before(:suite) do
  `bin/webpack`
end
@richseviora
Copy link

@richseviora richseviora commented Jun 3, 2017

@Jeremy-Walton Also observing the same.
@zacharyw thanks for the workaround!

I think the issue comes from WebPacker::Manifest#lookup, where it'll first attempt to find the manifest and then compile only if not found.

@gauravtiwari
Copy link
Member

@gauravtiwari gauravtiwari commented Jun 3, 2017

The idea was to keep this simple but if you need to recompile during tests just call compile on webpacker module:

before(:suite) do
  Webpacker.compile
end

screen shot 2017-06-03 at 22 56 09

@gauravtiwari
Copy link
Member

@gauravtiwari gauravtiwari commented Jun 3, 2017

The above will ensure that packs are recompiled and manifest is reloaded - ref:

def compile

@richseviora
Copy link

@richseviora richseviora commented Jun 3, 2017

It sounds like we save unnecessary compilation calls at the expense of introducing an implicit requirement that the user invokes WebPacker.compile (manually or before suite execution) whenever their JS code changes.

Is there a better way we could communicate this? I totally appreciate that we don't want to recompile the entire pack for every test run, but I think a lot of users are going to run into similar concerns.

@justin808
Copy link
Contributor

@justin808 justin808 commented Jun 4, 2017

@richseviora See #464. Also, I wrote the code for React on Rails that does this. We use the file mtimes.

@gauravtiwari
Copy link
Member

@gauravtiwari gauravtiwari commented Jun 4, 2017

Yepp, actually there was a PR made (#341 - https://github.com/rails/webpacker/pull/341/files#diff-23ac5b11daf84548b507c40511c4aa2eR21) to add very same feature but in the end went with more simpler and implicit solution.

Perhaps we should add that too so both options work

// cc @javan

@raldred
Copy link

@raldred raldred commented Jun 15, 2017

For me the cached version would be favourable. The speed of compilation whilst not an issue on CI is pretty frustrating developing locally. Particularly on a large react app.

@stevehanson
Copy link

@stevehanson stevehanson commented Jun 21, 2017

Compiling webpack in the before(:suite) every time I ran my tests was slowing me down quite a bit, so I ended up pulling @gauravtiwari's solution from his now-closed PR #341 into my codebase (slightly modified), and it's working great for me.

Using his approach, Webpack now only compiles if my source has changed. Thanks @gauravtiwari for that and for all of your diligent work on this gem. Would love to see your solution included into the Webpacker gem, as I think the functionality would be broadly useful.

For anyone looking for a quick fix, here is a gist of the code I ended up using, or check out PR #341.

@raldred
Copy link

@raldred raldred commented Jun 22, 2017

Thanks for sharing @stevehanson I wonder if #341 could be reopened and included as well. Both gives everyone the option depending on their use.

@gauravtiwari
Copy link
Member

@gauravtiwari gauravtiwari commented Jun 22, 2017

@stevehanson Thanks for sharing. Looks great 👍

@raldred @stevehanson Seems like there is real requirement for this so I will reopen #341 and we can review it together. It would be nice to have both options.

@kernow
Copy link

@kernow kernow commented Jun 22, 2017

I've just run into this issue too, a solution where webpack is recompiled only when something has changed before the test suite is run is whats needed IMHO. 👍 for something like #341

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet