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

Enhance asset symlinking for webpack assets #479

Merged
merged 2 commits into from
Aug 1, 2016

Conversation

justin808
Copy link
Member

@justin808 justin808 commented Jul 14, 2016

  • added test cases to spec/dummy
  • updated some gems

This change is Reviewable

@justin808
Copy link
Member Author

@alleycat-at-git Please review.

Dev mode works

foreman start -f Procfile.static

then go to http://localhost:3000/css_images_fonts

production does not

RAILS_ENV=production rake assets:precompile
rails s -e production

then go to http://localhost:3000/css_images_fonts

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 82.128% when pulling 43c2a52 on alleycat-at-git-alexey/replace_symlinks_copy into 20f98d9 on master.

@justin808
Copy link
Member Author

@alleycat-at-git it's a bad thing that tests are passing when this change is not working. Bonus points if you can see a way to test the correct behavior. Another issue is webpack 1.0 vs. 2.0. Possibly we should be doing this differently for webpack 2.0? and only supporting 2.0 with this PR?

@alexeuler
Copy link
Contributor

@justin808 I added a commit that fixes the problems. Didn't have time for bonuses - sorry ) Note that you have to use

RAILS_SERVE_STATIC_FILES=true rails s -e production

To run in production mode without nginx and friends


Reviewed 28 of 30 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


lib/react_on_rails/configuration.rb, line 12 [r2] (raw file):

    if @configuration.symlink_non_digested_assets_regex.present? &&
       @configuration.non_digested_assets_regex.blank?
      puts "[DEPRECATION] ReactOnRails: rename config.symlink_non_digested_assets_regex to "\

We have to be careful here - there are two configs - one in the gem and another in the app. While we have full control over the gem configuration, the point of risk is that app uses old config value - and that's where we put deprecation notice. Do we have access to app config via @configuration?


spec/dummy/config/secrets.yml, line 23 [r2] (raw file):

production:
  secret_key_base: <%= ENV["SECRET_KEY_BASE"].presence ||
   "8cd24e99fc1b68f6cd1592e4db8c88c2b508dbd12a37dd0cef184eb46aa5e3512f386d02c7a29971b8e34a9daa627642fc4a03b9c49e28f1943ea9028c91ba47" %>

Is this and intentional one?


spec/dummy/config/initializers/assets.rb, line 17 [r2] (raw file):

Dir.glob(Rails.root.join("client", "app", "assets", "**/")).each do |path|
  Rails.application.config.assets.paths << path.chop
end

This one includes not only folders, but subfolders as well


spec/dummy/config/initializers/assets.rb, line 30 [r2] (raw file):

  ]
Rails.application.config.assets.precompile += %w(*.png *.jpg *.jpeg *.gif *.tiff *.woff *.ttf *.eot
*.svg)

I have no idea why - but for some reason pipeline didn't touch those in custom dir. This line solved it


Comments from Reviewable

@alexeuler
Copy link
Contributor

Review status: all files reviewed at latest revision, 4 unresolved discussions.


spec/dummy/config/initializers/assets.rb, line 17 [r2] (raw file):

Previously, alleycat-at-git (Alexey) wrote…

This one includes not only folders, but subfolders as well

BTW like I said - I don't think it's a good idea to do this, that way you'll have x4 copies of the same asset - one generated by webpack, another with rails and x2 each for non-digested copies

Comments from Reviewable

@justin808
Copy link
Member Author

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


lib/react_on_rails/configuration.rb, line 12 [r2] (raw file):

Do we have access to app config via @configuration?
Yes


spec/dummy/config/secrets.yml, line 23 [r2] (raw file):

Previously, alleycat-at-git (Alexey) wrote…

Is this and intentional one?

yes, so testing in production does not require the key.

spec/dummy/config/initializers/assets.rb, line 17 [r2] (raw file):

Previously, alleycat-at-git (Alexey) wrote…

BTW like I said - I don't think it's a good idea to do this, that way you'll have x4 copies of the same asset - one generated by webpack, another with rails and x2 each for non-digested copies

We should change that then. We can use relative paths.

Comments from Reviewable

@robwise
Copy link
Contributor

robwise commented Jul 15, 2016

why are we switching from symlinks to copying? What about cleanup of stale asset files? This was handled elegantly via symlinks because they know when they are broken


Reviewed 28 of 30 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.


.rubocop.yml, line 54 [r2] (raw file):

Metrics/CyclomaticComplexity:
  Max: 9

that's one way to pass the linters


lib/react_on_rails/configuration.rb, line 67 [r2] (raw file):

      server_render_method: "",
      symlink_non_digested_assets_regex: /\.(png|jpg|jpeg|gif|tiff|woff|ttf|eot|svg)/,
      non_digested_assets_regex: /\.(png|jpg|jpeg|gif|tiff|woff|ttf|eot|svg)/,

we may also want to put .map type in here for source maps


lib/tasks/assets.rake, line 114 [r2] (raw file):

    .enhance do
      Rake::Task["react_on_rails:assets:symlink_non_digested_assets"].invoke
      Rake::Task["react_on_rails:assets:delete_broken_symlinks"].invoke

are you putting in any code to delete the files you've copied to the assets file if they are stale?


spec/dummy/client/webpack.client.base.config.js, line 63 [r2] (raw file):

  module: {
    loaders: [
      //{ test: /\.(woff2?|svg)$/, loader: 'url?limit=10000' },

??


spec/dummy/client/app/assets/fonts/OpenSans-Light.svg, line 1 [r2] (raw file):

<?xml version="1.0" standalone="no"?>

why are we adding 1000 lines of font svg code, we already have svgs we are testing, I would delete this


spec/dummy/config/initializers/assets.rb, line 17 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

We should change that then. We can use relative paths.

agree with @alleycat-at-git, this is not necessary, webpack automatically modifies references when it fingerprints assets

spec/dummy/config/initializers/assets.rb, line 30 [r2] (raw file):

Previously, alleycat-at-git (Alexey) wrote…

I have no idea why - but for some reason pipeline didn't touch those in custom dir. This line solved it

wait, why is this not reading from our config?

spec/dummy/config/initializers/react_on_rails.rb, line 86 [r2] (raw file):

  # For any asset matching this regex, a file is copied to the correct path to have a digest.
  # To disable creating digested assets, set this parameter to nil.
  config.non_digested_assets_regex = /\.(png|jpg|jpeg|gif|tiff|woff|ttf|eot|svg)/

should this be changed to an array of globs so that we can do *.png and get the files in nested folders?


Comments from Reviewable

@justin808
Copy link
Member Author

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.


.rubocop.yml, line 54 [r2] (raw file):

Previously, robwise (Rob Wise) wrote…

that's one way to pass the linters

@samnang This is because I want the config file to take the old or new value of a given property. Just adding the additional config value boosted this. Maybe I should set this override locally in the file, @robwise.

Comments from Reviewable

@justin808 justin808 force-pushed the alleycat-at-git-alexey/replace_symlinks_copy branch from 5a26a61 to 40c442f Compare July 18, 2016 09:48
@justin808 justin808 changed the title Change to copying assets for css modules Enhance asset symlinking for webpack assets Jul 18, 2016
@justin808
Copy link
Member Author

@robwise, @alleycat-at-git Please review, especially lib/tasks/assets.rake.


Review status: 9 of 24 files reviewed at latest revision, 10 unresolved discussions.


Comments from Reviewable

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.305% when pulling 40c442f on alleycat-at-git-alexey/replace_symlinks_copy into 20f98d9 on master.

@robwise
Copy link
Contributor

robwise commented Jul 18, 2016

Reviewed 21 of 25 files at r3.
Review status: all files reviewed at latest revision, 12 unresolved discussions.


docs/additional-reading/rails-assets.md, line 56 [r3] (raw file):

being processed by Webpack are just MD5's.
  • This is too verbose, (as is our entire manual). Every extra line in our docs makes people want to use our gem less. Keep that in mind. Everything added could be replaced with By default, Rails fingerprints all assets in production. Because you are not able to use Rails's asset helpers in your client code, ReactOnRails will automatically make un-fingerprinted versions of these files available. We recommend you still have webpack fingerprint these files itself so that you will still properly bust their respective caches when you change them.
  • We shouldn't log non-errors unless some sort of trace or debug mode is on, it's super annoying to have all of the noise we currently do from TestHelper during tests or seeing all of the ReactOnRails context messages in the JavaScript console. If we want to see that, we can turn it on, but most people probably don't want to see all of that (I know @alexfedoseev and I don't)
  • Not related to this file necessarily, but also we should only have one trace or debug mode, more than one is super confusing

lib/tasks/assets.rake, line 67 [r3] (raw file):

          end
        end
      end

ideally we would move all of this functionality into a PORO using the method_object pattern that can take the paths as arguments so that we can easily test that any of this works, then we simply call that object. Right now we have no coverage of anything happening here and we can't be sure we didn't just regress


Comments from Reviewable

@justin808
Copy link
Member Author

We shouldn't log non-errors unless some sort of trace or debug mode is on, it's super annoying to have all of the noise we currently do from TestHelper during tests or seeing all of the ReactOnRails context messages in the JavaScript console. If we want to see that, we can turn it on, but most people probably don't want to see all of that (I know @alexfedoseev and I don't)

Standard Rails does print the fingerprinting, so we should also print the symlinking. I'm OK with skipping the indication that files didn't change.

@justin808
Copy link
Member Author

We recommend you still have webpack fingerprint these files itself so that you will
still properly bust their respective caches when you change them.

I suspect that would be possible but unlikely. The file-loader does this MD5 name by default.

@justin808
Copy link
Member Author

Review status: all files reviewed at latest revision, 12 unresolved discussions.


docs/additional-reading/rails-assets.md, line 56 [r3] (raw file):

Previously, robwise (Rob Wise) wrote…

  • This is too verbose, (as is our entire manual). Every extra line in our docs makes people want to use our gem less. Keep that in mind. Everything added could be replaced with By default, Rails fingerprints all assets in production. Because you are not able to use Rails's asset helpers in your client code, ReactOnRails will automatically make un-fingerprinted versions of these files available. We recommend you still have webpack fingerprint these files itself so that you will still properly bust their respective caches when you change them.
  • We shouldn't log non-errors unless some sort of trace or debug mode is on, it's super annoying to have all of the noise we currently do from TestHelper during tests or seeing all of the ReactOnRails context messages in the JavaScript console. If we want to see that, we can turn it on, but most people probably don't want to see all of that (I know @alexfedoseev and I don't)
  • Not related to this file necessarily, but also we should only have one trace or debug mode, more than one is super confusing
Acknowledge this, but I don't want to change any of this now. In terms of verbose messages, Rails says what it's doing when compiling assets, so I want to do the same. There is only one ReactOnRails.trace mode.

lib/react_on_rails/configuration.rb, line 12 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Do we have access to app config via @configuration?
Yes

Done.

lib/tasks/assets.rake, line 114 [r2] (raw file):

Previously, robwise (Rob Wise) wrote…

are you putting in any code to delete the files you've copied to the assets file if they are stale?

There's no copying. We just add symlinks and delete any broken symlinks. In fact, Rails intentionally does not remove stale assets, as you can have older copy of the page still requesting an older resource.

lib/tasks/assets.rake, line 67 [r3] (raw file):

Previously, robwise (Rob Wise) wrote…

ideally we would move all of this functionality into a PORO using the method_object pattern that can take the paths as arguments so that we can easily test that any of this works, then we simply call that object. Right now we have no coverage of anything happening here and we can't be sure we didn't just regress

I agree. However, no energy to do that right now.

Comments from Reviewable

@justin808
Copy link
Member Author

Review status: all files reviewed at latest revision, 12 unresolved discussions.


spec/dummy/client/webpack.client.base.config.js, line 63 [r2] (raw file):

Previously, robwise (Rob Wise) wrote…

??

yep, duplicate line, fixed

spec/dummy/config/initializers/react_on_rails.rb, line 86 [r2] (raw file):

Previously, robwise (Rob Wise) wrote…

should this be changed to an array of globs so that we can do *.png and get the files in nested folders?

there's no nested folders -- webpack is all flat, as is the rails process.

.rubocop.yml, line 54 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

@samnang This is because I want the config file to take the old or new value of a given property. Just adding the additional config value boosted this. Maybe I should set this override locally in the file, @robwise.

Done.

spec/dummy/config/initializers/assets.rb, line 17 [r2] (raw file):

Previously, robwise (Rob Wise) wrote…

agree with @alleycat-at-git, this is not necessary, webpack automatically modifies references when it fingerprints assets

This has nothing to do with fingerprinting. This just tells rails to publish this directory. ![2016-07-18\_22-57-06.png](https://files.reviewableusercontent.io/images/reviewable/1118459/Djj89mtcYB4ZT230jJAjzMwq/2016-07-18_22-57-06.png)

spec/dummy/config/initializers/assets.rb, line 30 [r2] (raw file):

Previously, robwise (Rob Wise) wrote…

wait, why is this not reading from our config?

no change any more

Comments from Reviewable

@justin808 justin808 force-pushed the alleycat-at-git-alexey/replace_symlinks_copy branch from 40c442f to 98e2076 Compare July 19, 2016 09:02
@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.305% when pulling 98e2076 on alleycat-at-git-alexey/replace_symlinks_copy into 20f98d9 on master.

@alexeuler
Copy link
Contributor

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 13 unresolved discussions.


lib/tasks/assets.rake, line 27 [r4] (raw file):

        puts "React On Rails: Symlinking #{target_path} to #{symlink_path}"
        `cd #{ReactOnRails::assets_path} && ln -s #{target} #{symlink}`
      end

I think there were problems with this exact line. If we are in a root assets folder (public/assets) and asset in svg/1-fingerprint.svg then what it will do - it will make a symlink in public/assets/svg that will relatively point to svg/1-fingerprint.svg so effectively it will resolve to public/assets/svg/svg/1-fingerprint.svg which is non-existent.


Comments from Reviewable

@justin808
Copy link
Member Author

Review status: all files reviewed at latest revision, 13 unresolved discussions.


lib/tasks/assets.rake, line 27 [r4] (raw file):

Previously, alleycat-at-git (Alexey) wrote…

I think there were problems with this exact line. If we are in a root assets folder (public/assets) and asset in svg/1-fingerprint.svg then what it will do - it will make a symlink in public/assets/svg that will relatively point to svg/1-fingerprint.svg so effectively it will resolve to public/assets/svg/svg/1-fingerprint.svg which is non-existent.

No problem because no sub dirs are used. I tested this.

Comments from Reviewable

@robwise
Copy link
Contributor

robwise commented Jul 19, 2016

We recommend you still have webpack fingerprint these files itself so that you will
still properly bust their respective caches when you change them.

I suspect that would be possible but unlikely. The file-loader does this MD5 name by default.

You suspect that what it does by default would be possible but unlikely?


Review status: all files reviewed at latest revision, 13 unresolved discussions.


docs/additional-reading/rails-assets.md, line 56 [r3] (raw file):

Rails says what it's doing when compiling assets

Ok makes sense then

There is only one ReactOnRails.trace mode.

but there's trace, and then there's that other env var thing that you can set to YES as well isn't there?


lib/tasks/assets.rake, line 114 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

There's no copying. We just add symlinks and delete any broken symlinks. In fact, Rails intentionally does not remove stale assets, as you can have older copy of the page still requesting an older resource.

that comment was from a previous revision when we were still doing the change to copying

spec/dummy/config/initializers/react_on_rails.rb, line 86 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

there's no nested folders -- webpack is all flat, as is the rails process.

I talked with @alleycat-at-git, I think the confusion was he wanted to reference Rails-only assets from webpack as well, but I we need to allow for doing that because you wouldn't be busting the cache, you should just place said asset in your `client` dir instead

Comments from Reviewable

@robwise
Copy link
Contributor

robwise commented Jul 19, 2016

Review status: all files reviewed at latest revision, 10 unresolved discussions.


spec/dummy/config/initializers/assets.rb, line 17 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

This has nothing to do with fingerprinting. This just tells rails to publish this directory.
2016-07-18_22-57-06.png

@justin808 I think those comments were out of date, it seems to be fixed now

Comments from Reviewable

@alexeuler
Copy link
Contributor

:lgtm:


Comments from Reviewable

@justin808
Copy link
Member Author

Actually, @robwise is right. https://github.com/webpack/file-loader#filename-templates

HOWEVER, should we just document that we don't support that for now?

And just list that as an issue (and an exercise for the next react_on_rails contributor)?

@alexfedoseev Any opinion?


Review status: all files reviewed at latest revision, 10 unresolved discussions.


Comments from Reviewable

@justin808
Copy link
Member Author

BTW, I think the problem that @alleycat-at-git ran into in terms of using a file path for an image in JSX could have been solved by following one of the examples here: https://github.com/webpack/file-loader#examples.

Overall, I say we document this design decision and indicate that somebody that wants this can submit a PR to add this enhancement.


Review status: all files reviewed at latest revision, 10 unresolved discussions.


Comments from Reviewable

@robwise
Copy link
Contributor

robwise commented Jul 19, 2016

HOWEVER, should we just document that we don't support that for now?

It's already automatically supported and is currently being used on F&G in production. Whether or not webpack fingerprints its assets is its own business that it knows how to deal with via the file and url loaders, we just need to make sure Rails doesn't get in the way by fingerprinting them again (or more accurately, that we have symlinks without the rails fingerprint in the filename).

Since we can't rely on Rails's fingerprinting, it's definitely a best practice to have webpack fingerprint assets since that are being referenced by the file/url loaders so that their caches will be properly busted.


Review status: all files reviewed at latest revision, 8 unresolved discussions.


Comments from Reviewable

@justin808
Copy link
Member Author

@robwise by "that", I mean subdirectories. I'll see if there's an easy fix to allow that.


Review status: all files reviewed at latest revision, 8 unresolved discussions.


Comments from Reviewable

@justin808
Copy link
Member Author

@robwise @alleycat-at-git Easier to fix the issue than doc it.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.305% when pulling 745914b on alleycat-at-git-alexey/replace_symlinks_copy into 20f98d9 on master.

@justin808
Copy link
Member Author

I moved the rake code to a module.


# require "tmpdir"
# require "tempfile"

Copy link
Member Author

Choose a reason for hiding this comment

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

@samnang @mapreal19 What's the best way to write these tests given the dependencies on the Rails and ReactOnRails config values? I"m thinking of using tmpdir and tempfile, along with some sort of way to set the the global config values temporarily.

And if we make files for test in a tempdir, we don't need to cleanup, right?

@justin808
Copy link
Member Author

Review status: 22 of 27 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


spec/dummy/config/initializers/react_on_rails.rb, line 86 [r2] (raw file):

Previously, robwise (Rob Wise) wrote…

I talked with @alleycat-at-git, I think the confusion was he wanted to reference Rails-only assets from webpack as well, but I we need to allow for doing that because you wouldn't be busting the cache, you should just place said asset in your client dir instead

You could place the asset in the client dir, and access that from Rails.

@alleycat-at-git @robwise The key is to use the Webpack loader to get the proper file name that has the MD5/hash.

https://github.com/webpack/file-loader#examples


Comments from Reviewable

@justin808
Copy link
Member Author

Review status: 22 of 27 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


spec/dummy/client/app/assets/fonts/OpenSans-Light.svg, line 1 [r2] (raw file):

Previously, robwise (Rob Wise) wrote…

why are we adding 1000 lines of font svg code, we already have svgs we are testing, I would delete this

Done.

spec/dummy/config/initializers/assets.rb, line 17 [r2] (raw file):

Previously, robwise (Rob Wise) wrote…

@justin808 I think those comments were out of date, it seems to be fixed now

What is fixed now? I believe that Rails always intelligently would only include stuff in files like application.js or refereenced in the sass.

spec/dummy/config/initializers/assets.rb, line 30 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

no change any more

Done.

Comments from Reviewable

@mapreal19
Copy link
Member

Review status: 22 of 27 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


spec/react_on_rails/assets_precompile_spec.rb, line 6 [r6] (raw file):

Previously, justin808 (Justin Gordon) wrote…

@samnang @mapreal19 What's the best way to write these tests given the dependencies on the Rails and ReactOnRails config values? I"m thinking of using tmpdir and tempfile, along with some sort of way to set the the global config values temporarily.

And if we make files for test in a tempdir, we don't need to cleanup, right?

Maybe we could stub config values and have different contexts?

Yeah we shouldn't need to cleanup if using tempdir.

FYI this method could be useful http://ruby-doc.org/stdlib-2.0.0/libdoc/tmpdir/rdoc/Dir.html#method-c-mktmpdir


Comments from Reviewable

@robwise
Copy link
Contributor

robwise commented Jul 20, 2016

@justin808 oh, according to what Alexey told me that's already supported as well though; you just don't reference it in your JS code with the nesting because the symlinks flatten everything. But it's a bad practice to be putting assets you need for webpack into your rails assets, especially since this means you cannot cache them, so that doesn't make sense to me. Why wouldn't you just put it in your client folder like normal?

A totally different, more likely issue is that someone who has rails assets that have identical filenames but are in different folders, when we symlink to a flat structure, those symlinks will have identical filenames and therefore you will have some type of naming collision. This could be avoided one of two ways:

  • only symlink webpack assets
  • retain folder structure when symlinking

Review status: 22 of 27 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


spec/dummy/config/initializers/react_on_rails.rb, line 86 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

You could place the asset in the client dir, and access that from Rails.

@alleycat-at-git @robwise The key is to use the Webpack loader to get the proper file name that has the MD5/hash.

https://github.com/webpack/file-loader#examples

yep, been doing it for 8 months now

spec/dummy/config/initializers/assets.rb, line 17 [r2] (raw file):

Dir.glob(Rails.root.join("client", "app", "assets", "**/")).each do |path|
Rails.application.config.assets.paths << path.chop
end

this got taken back out, the comments were on that ^. so it's fixed now


Comments from Reviewable

@justin808
Copy link
Member Author

Review status: 22 of 27 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


spec/react_on_rails/assets_precompile_spec.rb, line 6 [r6] (raw file):

Previously, mapreal19 (Mario Pérez) wrote…

Maybe we could stub config values and have different contexts?

Yeah we shouldn't need to cleanup if using tempdir.

FYI this method could be useful http://ruby-doc.org/stdlib-2.0.0/libdoc/tmpdir/rdoc/Dir.html#method-c-mktmpdir

@samnang @mapreal19 @robwise @aaronvb @jbhatab @alleycat-at-git

I'm torn over shipping this absolutely needed bug fix without the proper tests, as we want to display best practices. However, I'm beyond swamped right now!

@samnang @mapreal19 any example specs that point to how we could stub out the directories for the Rails and ReactOnRails config values, and then we could create temp files, and then verify that the new methods do the right thing?


Comments from Reviewable

@justin808 justin808 force-pushed the alleycat-at-git-alexey/replace_symlinks_copy branch from 277f33b to 0d75282 Compare July 21, 2016 06:57
@samnang
Copy link
Contributor

samnang commented Jul 21, 2016

Reviewed 1 of 4 files at r6.
Review status: 22 of 27 files reviewed at latest revision, 9 unresolved discussions, some commit checks broke.


spec/react_on_rails/assets_precompile_spec.rb, line 6 [r6] (raw file):

Previously, justin808 (Justin Gordon) wrote…

@samnang @mapreal19 @robwise @aaronvb @jbhatab @alleycat-at-git

I'm torn over shipping this absolutely needed bug fix without the proper tests, as we want to display best practices. However, I'm beyond swamped right now!

@samnang @mapreal19 any example specs that point to how we could stub out the directories for the Rails and ReactOnRails config values, and then we could create temp files, and then verify that the new methods do the right thing?

@justin808 When I look at the code, I see file system is really couple to the implementation, so I think the only way we need touch file system api, each we use tmpfile/tmpdir, https://github.com/fakefs/fakefs, or https://github.com/simonc/memfs. Then you can away from coupling with the real file system path. /cc @mapreal19

.rubocop.yml, line 54 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Done.

Not sure, something that should be standard should be in this file. An overrided value that's for personal preference which global doesn't enforce could go in local config that is ignore by your global git ignore.

Comments from Reviewable

@justin808 justin808 force-pushed the alleycat-at-git-alexey/replace_symlinks_copy branch from 0d75282 to fcde037 Compare July 21, 2016 09:33
@coveralls
Copy link

Coverage Status

Coverage decreased (-4.6%) to 77.749% when pulling fcde037 on alleycat-at-git-alexey/replace_symlinks_copy into e6afa98 on master.

@justin808
Copy link
Member Author

@mapreal19 @samnang last commit should make testings much easier. Agree?


Review status: 22 of 27 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


Comments from Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.8%) to 77.551% when pulling 1e7ab23 on alleycat-at-git-alexey/replace_symlinks_copy into e6afa98 on master.

@justin808
Copy link
Member Author

I'll be pulling #490 in for the test. Thanks @dzirtusss .

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 82.27% when pulling 48cb445 on alleycat-at-git-alexey/replace_symlinks_copy into e6afa98 on master.

@justin808 justin808 force-pushed the alleycat-at-git-alexey/replace_symlinks_copy branch from 48cb445 to 06ebd42 Compare August 1, 2016 00:55
justin808 and others added 2 commits July 31, 2016 14:57
* Better messages when creating symlinks
* Updated documentation
* Enhanced example
* Support subdirectories with webpack assets
* Move logic for assets code to service object
* Using defaults of the env settings or else values for directories and
  regexp can be provided.
symlink tests with tempfs
@justin808 justin808 force-pushed the alleycat-at-git-alexey/replace_symlinks_copy branch from 06ebd42 to a6e35fe Compare August 1, 2016 00:57
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 82.27% when pulling a6e35fe on alleycat-at-git-alexey/replace_symlinks_copy into cdb246b on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 82.27% when pulling a6e35fe on alleycat-at-git-alexey/replace_symlinks_copy into cdb246b on master.

@justin808 justin808 merged commit 46ecf59 into master Aug 1, 2016
@justin808 justin808 deleted the alleycat-at-git-alexey/replace_symlinks_copy branch August 1, 2016 01:42
eacaps added a commit to eacaps/react_on_rails that referenced this pull request Aug 2, 2016
commit 7d4e7fe
Merge: cb9fab4 3c0e6d9
Author: eacaps <[email protected]>
Date:   Mon Aug 1 11:46:35 2016 -0400

    Merge branch 'feature/no-request-context' of github.com:eacaps/react_on_rails into feature/no-request-context

commit cb9fab4
Author: eacaps <[email protected]>
Date:   Thu Jul 21 13:23:51 2016 -0400

    allow component rendering in contexts without requests

    allow custom_context even without request

    added broken test

    added fixes to allow actionmailer test to pass

    cleaned up rubocop warnings

    resolved some reviewed suggestions

    updated with proper handles for inMailer

    added inMailer to test

    added inMailer checks for test

    cleaned up rubocop suggestions

    a few final minor tweaks

commit 3c0e6d9
Merge: 5637162 63436ab
Author: eacaps <[email protected]>
Date:   Mon Aug 1 10:44:59 2016 -0400

    Merge branch 'master' into feature/no-request-context

commit 5637162
Author: eacaps <[email protected]>
Date:   Mon Aug 1 10:35:40 2016 -0400

    a few final minor tweaks

commit 63436ab
Merge: b4cdfd2 d694df8
Author: Justin Gordon <[email protected]>
Date:   Sun Jul 31 23:01:59 2016 -1000

    Merge pull request shakacode#491 from samphilipd/master

    Add support for single digit version strings, closes shakacode#489

commit d694df8
Author: Sam Davies <[email protected]>
Date:   Wed Jul 27 10:52:49 2016 +0100

    Add support for single digit version strings, closes shakacode#489

commit b4cdfd2
Author: Justin Gordon <[email protected]>
Date:   Sun Jul 31 21:25:57 2016 -1000

    Update README.md

commit 8824b88
Merge: e449d84 442dcd4
Author: Justin Gordon <[email protected]>
Date:   Sun Jul 31 21:25:35 2016 -1000

    Merge pull request shakacode#503 from markpenovich/master

    fixed spelling error

commit 442dcd4
Author: Mark Penovich <[email protected]>
Date:   Sun Jul 31 23:47:20 2016 -0500

    fixed spelling error in readme

commit e449d84
Merge: 95efecd 8cec9cf
Author: Justin Gordon <[email protected]>
Date:   Sun Jul 31 15:49:11 2016 -1000

    Merge pull request shakacode#497 from shakacode/justin808-skip-docker

    Remove docker from CI tests .travis.yml

commit 95efecd
Merge: 46ecf59 ef08742
Author: Justin Gordon <[email protected]>
Date:   Sun Jul 31 15:48:51 2016 -1000

    Merge pull request shakacode#502 from shakacode/move-contributing-to-top-level

    Move CONTRIBUTING.MD to project top level

commit ef08742
Author: Justin Gordon <[email protected]>
Date:   Sun Jul 31 15:06:11 2016 -1000

    Move CONTRIBUTING.MD to project top level

    This seems to make it show more prominently when making new issues or
    PRs.

commit 46ecf59
Merge: cdb246b a6e35fe
Author: Justin Gordon <[email protected]>
Date:   Sun Jul 31 15:42:20 2016 -1000

    Merge pull request shakacode#479 from shakacode/alleycat-at-git-alexey/replace_symlinks_copy

    * Enhancements to webpack asset preparation
    * Better messages when creating symlinks
    * Updated documentation
    * Enhanced example
    * Support subdirectories with webpack assets
    * Move logic for assets code to service object
    * Using defaults of the env settings or else values for directories and
      regexp can be provided.

commit 8cec9cf
Author: Justin Gordon <[email protected]>
Date:   Fri Jul 29 15:34:10 2016 -1000

    Remove Docker from setup

    * Update .travis.yml
    * Remove Dockerfile_tests and docker-compose.yml

commit a6e35fe
Author: dzirtusss <[email protected]>
Date:   Tue Jul 26 18:43:20 2016 +0300

    Update assets_precompile_spec.rb

    symlink tests with tempfs

commit 07a6e48
Author: Justin Gordon <[email protected]>
Date:   Sun Jul 17 23:23:03 2016 -1000

    Enhancements to webpack asset preparation

    * Better messages when creating symlinks
    * Updated documentation
    * Enhanced example
    * Support subdirectories with webpack assets
    * Move logic for assets code to service object
    * Using defaults of the env settings or else values for directories and
      regexp can be provided.

commit cdb246b
Author: Justin Gordon <[email protected]>
Date:   Sat Jul 30 12:30:33 2016 -1000

    Doc Fixes (shakacode#499)

    * Update node-server-rendering.md
    * Update README.md

commit 1e4c0ed
Author: Justin Gordon <[email protected]>
Date:   Sat Jul 30 11:16:25 2016 -1000

    Update README.md

commit 13f2cf0
Author: Justin Gordon <[email protected]>
Date:   Thu Jul 28 13:38:29 2016 -1000

    Update server-rendering-tips.md (shakacode#494)

    Update server-rendering-tips.md and README.md

commit 0604006
Merge: 8ca86ed e69480a
Author: Justin Gordon <[email protected]>
Date:   Wed Jul 27 22:14:17 2016 -1000

    Merge pull request shakacode#492 from cubadomingo/patch-1

    Fixes typo

commit e69480a
Author: Devin Osorio <[email protected]>
Date:   Wed Jul 27 22:34:13 2016 -0400

    Fixes typo

commit 566d62f
Author: eacaps <[email protected]>
Date:   Mon Jul 25 12:59:44 2016 -0400

    cleaned up rubocop suggestions

commit e24fb9f
Author: eacaps <[email protected]>
Date:   Mon Jul 25 11:28:56 2016 -0400

    added inMailer checks for test

commit eb6e700
Author: eacaps <[email protected]>
Date:   Mon Jul 25 11:14:41 2016 -0400

    added inMailer to test

commit 8d34353
Author: eacaps <[email protected]>
Date:   Mon Jul 25 10:41:35 2016 -0400

    updated with proper handles for inMailer

commit 52f0cbb
Merge: 840bc62 8ca86ed
Author: eacaps <[email protected]>
Date:   Mon Jul 25 10:27:23 2016 -0400

    Merge branch 'master' into feature/no-request-context

commit 8ca86ed
Merge: 9a8b54f 0257cae
Author: Justin Gordon <[email protected]>
Date:   Fri Jul 22 21:35:57 2016 -1000

    Merge pull request shakacode#487 from jooohn/fix/typo-in-readme

    fix(typo) remove duplicated word in readme

commit 0257cae
Author: jooohn <[email protected]>
Date:   Sat Jul 23 13:12:37 2016 +0900

    fix(typo) remove duplicated word in readme

commit 840bc62
Author: eacaps <[email protected]>
Date:   Fri Jul 22 09:57:28 2016 -0400

    resolved some reviewed suggestions

commit 9b958fb
Merge: 6fdb73a 9a8b54f
Author: eacaps <[email protected]>
Date:   Fri Jul 22 09:56:35 2016 -0400

    Merge branch 'master' into feature/no-request-context

commit 9a8b54f
Merge: e6afa98 4890486
Author: Justin Gordon <[email protected]>
Date:   Thu Jul 21 17:42:32 2016 -1000

    Merge pull request shakacode#483 from shakacode/justin808-inaccurate-build-test-message

    Update ensure_assets_compiled.rb

commit 6fdb73a
Author: eacaps <[email protected]>
Date:   Thu Jul 21 16:35:42 2016 -0400

    cleaned up rubocop warnings

commit d99fc36
Author: eacaps <[email protected]>
Date:   Thu Jul 21 14:46:20 2016 -0400

    added fixes to allow actionmailer test to pass

commit 05d3144
Author: eacaps <[email protected]>
Date:   Thu Jul 21 14:31:19 2016 -0400

    added broken test

commit 610906e
Author: eacaps <[email protected]>
Date:   Thu Jul 21 13:25:42 2016 -0400

    allow custom_context even without request

commit 1e9d58c
Author: eacaps <[email protected]>
Date:   Thu Jul 21 13:23:51 2016 -0400

    allow component rendering in contexts without requests

commit 4890486
Author: Justin Gordon <[email protected]>
Date:   Wed Jul 20 19:55:39 2016 -1000

    Update ensure_assets_compiled.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants