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

Enable webpack-dev-server with SSR #1173

Merged
merged 3 commits into from
Dec 6, 2018

Conversation

Judahmeek
Copy link
Contributor

@Judahmeek Judahmeek commented Dec 5, 2018

This change is Reviewable

Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Judahmeek Judahmeek force-pushed the judahmeek/ssr-webpack-dev-server branch 4 times, most recently from e58d459 to 1078624 Compare December 5, 2018 07:00
Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Left comment

Reviewed 5 of 5 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Judahmeek)


lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb, line 104 at r2 (raw file):

        def read_bundle_js_code
          server_js_file_path = ReactOnRails::Utils.server_bundle_js_file_path
          server_js_uri = URI.parse(server_js_file_path)

We have another method to see if the server_js_file_path contains a leading http.

Why not check first and branch off that logic?

@Judahmeek Judahmeek force-pushed the judahmeek/ssr-webpack-dev-server branch 2 times, most recently from 3b5abfd to 2505b0b Compare December 5, 2018 09:49
@Judahmeek Judahmeek force-pushed the judahmeek/ssr-webpack-dev-server branch from 2505b0b to ee1720b Compare December 5, 2018 09:51
Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Judahmeek)


lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb, line 107 at r3 (raw file):

          server_js_file = ReactOnRails::Utils.server_bundle_js_file_path
          if ReactOnRails::Utils.server_bundle_path_is_http?
            Net::HTTP.get(URI.parse(server_js_file)).force_encoding("UTF-8")

this is a good change

However, can we add a small comment on the force_encoding?

Maybe break the line into 2 so each intermediate value has a descriptive variable name?

raw_string_value_of_bundle = Net::HTTP.get(URI.parse(server_js_file))

// This is necessary because blah blah
raw_string_value_of_bundle.force_encoding("UTF-8")

Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Left a comment.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Judahmeek)

Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Judahmeek)


lib/react_on_rails/utils.rb, line 99 at r4 (raw file):

    def self.bundle_js_file_path(bundle_name)
      if ReactOnRails::WebpackerUtils.using_webpacker? && bundle_name != "manifest.json"
        ReactOnRails::WebpackerUtils.bundle_js_uri_from_webpacker(bundle_name)

webpacker returns either a file path or a URL so renamed to uri to reflect that

@justin808 justin808 merged commit fa292c7 into master Dec 6, 2018
@justin808 justin808 deleted the judahmeek/ssr-webpack-dev-server branch December 6, 2018 04:27
@coveralls
Copy link

coveralls commented Dec 8, 2018

Coverage Status

Coverage remained the same at ?% when pulling 36134a5 on judahmeek/ssr-webpack-dev-server into b4d3763 on master.

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.

3 participants