Add Automated Files System Based Pack Generation #1455

pulkitkkr · 2022-05-30T22:02:30Z

Summary

This PR adds

react_on_rails:generate_packs rake task
updates to View Helpers to generate component packs if not present.
add auto_load_bundle config option
add tests for packs generation

Detail

It has been a mundane task to create entry files, e.g.: client_entry.js inside the packs directory for a long time. The sole purpose of this file was to import a component and register it using the ReactOnRails.register API for usage in rails views.

The PR automates this process by introducing the concept of react on rail components. Any .jsx file components inside the ror_components directory will be detected and registered automatically for the usage on Rails View using the new helper react_component_with_bundle.

The name of the directory can be configured in src/config/initializers/react_on_rails.rb by setting

config.components_directory = "new_directory_name"

This change is

justin808 · 2022-06-15T09:49:50Z

@pulkitkkr what's remaining?

lib/react_on_rails/packs_generator.rb

justin808 · 2022-07-05T21:20:50Z

Needs documentation.

Needs tests.

Can we have the spec/dummy app show this?

justin808 · 2022-07-07T09:58:08Z

@pulkitkkr what's remaining?

lib/react_on_rails/helper.rb

lib/react_on_rails/configuration.rb

lib/react_on_rails/packs_generator.rb

alexeyr-ci1 · 2022-07-12T15:27:34Z

The tests look fine. If the .jsx files are supposed to be empty, maybe make it clear by making them consist of # Empty test component comment?

alexeyr-ci1 · 2022-07-12T15:31:46Z

Also is load_bundle a good name for the added option? It doesn't look particularly related to components. But I am not certain what should replace it, if anything. load_component_bundle? load_component_pack?

justin808 · 2022-07-12T22:15:38Z

@alexeyr-ci1 how about

auto_load_bundle

which signifies that no additional work is required to ensure the right bundles load on a given Rails view.

This works by automatically calling append_javascript_pack_tag and append_stylesheet_pack_tag whenever react_component and react_component_hash are used.

justin808 · 2022-07-12T22:31:52Z

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb

+  # FILE SYSTEM BASED COMPONENT REGISTRY
+  ################################################################################
+  # components_directory is the directory that is used to automatically detect and register components for the usage on
+  # the rails view. By Default any component inside `ror_components` directory is registered for the usage on rails view


default is nil

if defined:

assets precompile will automatically run the rake task

for development: react_component view for an auto_load_bundle == true will confirm that the generated file inside the components_directory, generated/{component_name} exists in the packs (entries) directory exists. If not, run the rake task to build, and log a message to the console that the generated files were rebuilt. If directory of generated is empty, build all of the generated. Or else just build the single generated file.

For non-development, raise an error suggesting that the rake task needs to run. This will mainly apply to a test run.

alexeyr-ci1 · 2022-07-14T07:16:13Z

lib/tasks/generate_packs.rake

+
+namespace :react_on_rails do
+  desc <<~DESC
+    If there is a jsx file inside config.components_directory, this command generates corresponding packs and#{' '}


#{' '} is probably accidental.

alexeyr-ci1 · 2022-07-14T07:17:34Z

lib/tasks/generate_packs.rake

+namespace :react_on_rails do
+  desc <<~DESC
+    If there is a jsx file inside config.components_directory, this command generates corresponding packs and#{' '}
+    registers the component for usage with react_component_with_bundle helper.


Actually, is this helper added? Because I can't see it anywhere in changes. If it isn't added because it was decided to take another approach, please change this and also update the PR description.

alexeyr-ci1 · 2022-07-14T07:20:44Z

which signifies that no additional work is required to ensure the right bundles load on a given Rails view.

But then do we ever want it to be false? Or at least do we want it to be false by default?

Unless it's just for backward compatibility, in which case we can document that the default will be true in the next major version.

lib/react_on_rails/packs_generator.rb

docs/guides/file-system-based-automated-bundle-generation.md

pulkitkkr · 2022-07-21T21:52:03Z

Address Docs related concerns @alexeyr

docs/guides/file-system-based-automated-bundle-generation.md

pulkitkkr · 2022-08-17T02:50:00Z

docs/guides/file-system-based-automated-bundle-generation.md line 110 at r21 (raw file):

Previously, justin808 (Justin Gordon) wrote…

We need to mention what's happening under the hood. I think that will make the transition clear from append_javascript_pack_tag, append_stylesheet_pack_tag to the new format.

I do not favour giving in-depth details about the internal functioning, as it will make it difficult for beginners to understand. Also, To understand the internals, the reader must be familiar with the shakapacker API. Think about someone who just started using React on Rails, following the example to configure and get created. We as creators are familiar with append_javascript_pack_tag and append_stylesheet_pack_tag; It's not something that many people will be familiar with.

If you still feel so, I can add details.

justin808

Reviewable status: 2 of 32 files reviewed, 56 unresolved discussions (waiting on @alexeyr, @alexeyr-ci1, @justin808, and @pulkitkkr)

docs/guides/file-system-based-automated-bundle-generation.md line 110 at r21 (raw file):

Previously, pulkitkkr (Pulkit) wrote…

I do not favour giving in-depth details about the internal functioning, as it will make it difficult for beginners to understand. Also, To understand the internals, the reader must be familiar with the shakapacker API. Think about someone who just started using React on Rails, following the example to configure and get created. We as creators are familiar with append_javascript_pack_tag and append_stylesheet_pack_tag; It's not something that many people will be familiar with.

If you still feel so, I can add details.

it's good to let folks know that are migrating. You can preface the explanatiion

justin808

Reviewable status: 2 of 32 files reviewed, 55 unresolved discussions (waiting on @alexeyr, @alexeyr-ci1, @justin808, and @pulkitkkr)

docs/guides/file-system-based-automated-bundle-generation.md line 131 at r21 (raw file):

Previously, pulkitkkr (Pulkit) wrote…

If just client, It's recommended to use client in the name. Otherwise, the component without client/server in the filename gets registered for both server and client.

ok

justin808

Reviewed 1 of 9 files at r1, 3 of 16 files at r4, 9 of 25 files at r25, all commit messages.
Reviewable status: 15 of 32 files reviewed, 78 unresolved discussions (waiting on @alexeyr, @alexeyr-ci1, @justin808, and @pulkitkkr)

README.md line 54 at r25 (raw file):

2. Tight integration with [shakapacker](https://github.com/shakacode/shakapacker) (or it's predecessor [rails/webpacker](https://github.com/rails/webpacker)).
3. Server-Side Rendering (SSR), often used for SEO crawler indexing and UX performance.
4. [Automated optimized entry-point creation and bundle inclusion when placing a component on a page. With this feature, you no longer need to configure javascript_pack_tags and stylesheet_pack_tags on your layouts based on what’s shown. “It just works!”](https://www.shakacode.com/react-on-rails/docs/guides/file-system-based-automated-bundle-generation.md)

back ticks

`javascript_pack_tags` and `stylesheet_pack_tags`

Code quote:

javascript_pack_tags and stylesheet_pack_tags

README.md line 58 at r25 (raw file):

6. [Internationalization (I18n) and (localization)](https://www.shakacode.com/react-on-rails/docs/guides/i18n)
7. A supportive community. This [web search shows how live public sites are using React on Rails](https://publicwww.com/websites/%22react-on-rails%22++-undeveloped.com+depth%3Aall/).
8. [ReScript Support](https://github.com/shakacode/rescript-react-on-rails-example).

Please don't convert to numbers. Unnecessary.

docs/guides/file-system-based-automated-bundle-generation.md line 105 at r25 (raw file):

  │   └── Bar
  │   │ └── ...
  │   │ └── ror_components          # configured as `components_directory`

I think we're changing that name because it's not just one directory.

docs/guides/file-system-based-automated-bundle-generation.md line 131 at r25 (raw file):

<%= foo_component_one_data["componentHtml"] %>

The default value of the `auto_load_bundle` parameter can be specified by setting `config.auto_load_bundle` in `config/initializers/react_on_rails.rb` and thus removed from each call to `react_component`.

Code quote:

the default value of the `auto_load_bundle` parameter can be specified by setting `config.auto_load_bundle` in `config/initializers/react_on_rails.rb`.

lib/react_on_rails/configuration.rb line 42 at r25 (raw file):

      same_bundle_for_client_and_server: false,
      i18n_output_format: nil,
      components_directory: DEFAULT_COMPONENTS_DIR

components_dir_name

DEFAULT_COMPONENTS_DIR_NAME

lib/react_on_rails/configuration.rb line 56 at r25 (raw file):

                  :i18n_dir, :i18n_yml_dir, :i18n_output_format,
                  :server_render_method, :random_dom_id, :auto_load_bundle,
                  :same_bundle_for_client_and_server, :rendering_props_extension, :components_directory

components_dir_name

lib/react_on_rails/packs_generator.rb line 90 at r2 (raw file):

Previously, pulkitkkr (Pulkit) wrote…

Done.

I prefer parens consistently used except where idiomatic, like put 'abc', @alexeyr-ci1 @pulkitkkr

lib/react_on_rails/packs_generator.rb line 68 at r5 (raw file):

        ReactOnRails.register({#{components_to_register.join(",\n")}});
        /* eslint-enable */

no need for eslint clutter

lib/react_on_rails/packs_generator.rb line 84 at r5 (raw file):

    def self.generated_server_bundle_file_path
      generated_server_bundle_file_name = component_name defined_server_bundle_file_path.sub(".js", "-generated.js")

@pulkitkkr

What if somebody used .ts, or.jsx or .tsx?

How about.

Get the file suffix
Substring without file suffix length, then append the -generated and the old suffix?

lib/react_on_rails/packs_generator.rb line 105 at r5 (raw file):

    def self.source_entry_path
      ReactOnRails::WebpackerUtils.webpacker_source_entry_path

no need for method

inline

lib/react_on_rails/packs_generator.rb line 161 at r5 (raw file):

      source_path = ReactOnRails::WebpackerUtils.webpacker_source_path

      "#{source_path}/**/#{components_directory}"

      "#{source_path}/**/#{components_dir_name}"

Code quote:

      "#{source_path}/**/#{components_directory}"

lib/react_on_rails/packs_generator.rb line 165 at r5 (raw file):

    def self.components_directory
      ReactOnRails.configuration.components_directory

why have methods like this?

need to expose this for the api of packs_generator?

why not have some private methods and not have only static methods?

just have a factory method that creates an instance

lib/react_on_rails/packs_generator.rb line 168 at r5 (raw file):

    end

    def self.prepend_to_file_if_not_present(file, str)

a better var name for str is text_to_prepend

see how it reads below

str is a meaningless name

lib/react_on_rails/packs_generator.rb line 173 at r5 (raw file):

        contents = fd.read
        content += contents
      end

What is the point of ^^^^.

contents = File.read(file)

lib/react_on_rails/packs_generator.rb line 175 at r5 (raw file):

      end

      return if content.start_with? str

What if the import is not the first line? then put the import in twice?

plus method name says "not_present" versus "not_first"

lib/react_on_rails/packs_generator.rb line 179 at r5 (raw file):

      File.open(file, "w") do |fd|
        fd.write str + content
      end

and yet another way to write a file?

What's wrong with File.write(file, content)?

lib/react_on_rails/packs_generator.rb line 14 at r25 (raw file):

    def self.generate
      return unless components_directory.present?

    return unless components_dir_name.present?

Code quote:

    return unless components_directory.present?

lib/react_on_rails/packs_generator.rb line 41 at r25 (raw file):

      f = File.new(output_path, "w")
      f.puts content
      f.close

Why use 3 lines

File.write(output_path, content)

lib/react_on_rails/packs_generator.rb line 49 at r25 (raw file):

      registered_component_name = component_name file_path
      <<~FILE_CONTENT
        /* eslint-disable */

Why not let people just exclude this directory from eslint?

lib/react_on_rails/packs_generator.rb line 61 at r25 (raw file):

      f = File.new(generated_server_bundle_file_path, "w")
      f.puts generated_server_pack_file_content
      f.close

One line ^^^^, not 3

lib/react_on_rails/packs_generator.rb line 69 at r25 (raw file):

    def self.generated_server_pack_file_content
      common_components_for_server_bundle = common_component_to_path.delete_if { |k| server_component_to_path.key? k }
      component_for_server_registration_to_path = common_components_for_server_bundle.merge server_component_to_path

I hate excluding parens for syntax like this.

yes, rubocop has no rule for this.

lib/react_on_rails/packs_generator.rb line 113 at r25 (raw file):

      server_bundle_file_name = ReactOnRails.configuration.server_bundle_js_file

      Dir.glob("#{source_entry_path}/**/#{server_bundle_file_name}").first

This is not right. There is a method that gives this exact file path.

          server_js_file = ReactOnRails::Utils.server_bundle_js_file_path

lib/react_on_rails/packs_generator.rb line 126 at r25 (raw file):

    def self.relative_component_path_from_generated_pack(ror_component_path)
      component_file_path = Pathname.new ror_component_path
      generated_pack_path = Pathname.new generated_pack_path ror_component_path

no parens?

And I especially don't understand this line. It seems like a syntax error! or You're missing a comma? and parens?

      generated_pack_path = Pathname.new generated_pack_path ror_component_path

lib/react_on_rails/packs_generator.rb line 135 at r25 (raw file):

      to_path = Pathname.new to

      # TODO: Debug Relative Path always has extra '../'

remove todo?

lib/react_on_rails/test_helper/webpack_assets_status_checker.rb line 29 at r25 (raw file):

      def stale_generated_webpack_files
        stale_generated_files client_files

I hate skipping the parens.

lib/react_on_rails/test_helper/webpack_assets_status_checker.rb line 97 at r25 (raw file):

      def components_search_path
        "#{source_path}/**/#{ReactOnRails.configuration.components_directory}"

        "#{source_path}/**/#{ReactOnRails.configuration.components_dir_name}"

Code quote:

        "#{source_path}/**/#{ReactOnRails.configuration.components_directory}"

lib/tasks/generate_packs.rake line 5 at r25 (raw file):

namespace :react_on_rails do
  desc <<~DESC
    If there is a jsx file inside config.components_directory, this command generates corresponding packs.

    If there is a file inside any directory matching config.components_dir_name, this command generates corresponding packs.

Note, don't mention jsx in case tsx is used.

I'm not sure what would happen with ReScript.

Code quote:

    If there is a jsx file inside config.components_directory, this command generates corresponding packs.

pulkitkkr

Reviewable status: 15 of 32 files reviewed, 78 unresolved discussions (waiting on @alexeyr, @alexeyr-ci1, @justin808, and @pulkitkkr)

README.md line 54 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

back ticks

`javascript_pack_tags` and `stylesheet_pack_tags`

Done.

README.md line 58 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Please don't convert to numbers. Unnecessary.

Done.

docs/guides/file-system-based-automated-bundle-generation.md line 105 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I think we're changing that name because it's not just one directory.

Done.

docs/guides/file-system-based-automated-bundle-generation.md line 131 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

The default value of the `auto_load_bundle` parameter can be specified by setting `config.auto_load_bundle` in `config/initializers/react_on_rails.rb` and thus removed from each call to `react_component`.

Done.

lib/react_on_rails/configuration.rb line 42 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

components_dir_name

DEFAULT_COMPONENTS_DIR_NAME

Updated to components_subdirectory and DEFAULT_COMPONENTS_SUBDIRECTORY

lib/react_on_rails/configuration.rb line 56 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

components_dir_name

updated to components_subdirectory

lib/react_on_rails/packs_generator.rb line 68 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

no need for eslint clutter

REMOVED

lib/react_on_rails/packs_generator.rb line 161 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

      "#{source_path}/**/#{components_dir_name}"

Done.

"#{source_path}/**/#{components_subdirectory}"

lib/react_on_rails/packs_generator.rb line 168 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

a better var name for str is text_to_prepend

see how it reads below

str is a meaningless name

Done.

lib/react_on_rails/packs_generator.rb line 173 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

What is the point of ^^^^.
contents = File.read(file) 

Done.

lib/react_on_rails/packs_generator.rb line 175 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

What if the import is not the first line? then put the import in twice?

plus method name says "not_present" versus "not_first"

Done.

lib/react_on_rails/packs_generator.rb line 179 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

and yet another way to write a file?

What's wrong with File.write(file, content)?

Done.

lib/react_on_rails/packs_generator.rb line 14 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

    return unless components_dir_name.present?

return unless components_subdirectory.present?

lib/react_on_rails/packs_generator.rb line 41 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Why use 3 lines
File.write(output_path, content)

Done.

lib/react_on_rails/packs_generator.rb line 49 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Why not let people just exclude this directory from eslint?

Done.

lib/react_on_rails/packs_generator.rb line 61 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

One line ^^^^, not 3

Done.

lib/react_on_rails/packs_generator.rb line 113 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

This is not right. There is a method that gives this exact file path.
          server_js_file = ReactOnRails::Utils.server_bundle_js_file_path

Done.

lib/react_on_rails/packs_generator.rb line 135 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

remove todo?

Done.

lib/react_on_rails/test_helper/webpack_assets_status_checker.rb line 97 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

        "#{source_path}/**/#{ReactOnRails.configuration.components_dir_name}"

Done.

lib/tasks/generate_packs.rake line 5 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

    If there is a file inside any directory matching config.components_dir_name, this command generates corresponding packs.
Note, don't mention jsx in case tsx is used.

I'm not sure what would happen with ReScript.

Done.

justin808

Reviewed 1 of 12 files at r3, 1 of 18 files at r11, 2 of 23 files at r18, 1 of 23 files at r20, 1 of 25 files at r23, 11 of 25 files at r25, 9 of 10 files at r26, all commit messages.
Reviewable status: 31 of 32 files reviewed, 53 unresolved discussions (waiting on @alexeyr, @alexeyr-ci1, @justin808, @pulkitkkr, and @tomdracz)

docs/api/view-helpers-api.md line 30 at r26 (raw file):

  - **props:** Ruby Hash which contains the properties to pass to the react object, or a JSON string. If you pass a string, we'll escape it for you.
  - **prerender:** enable server-side rendering of a component. Set to false when debugging!
  - **auto_load_bundle:** will automatically load bundle for component by calling `append_javascript_pack_tag` and `append_stylesheet_pack_tag` under the hood.

- **auto_load_bundle:** will automatically load the bundle for component by calling `append_javascript_pack_tag` and `append_stylesheet_pack_tag` under the hood.

the bundle

docs/guides/configuration.md line 160 at r26 (raw file):

  # components_subdirectory is the name of the directory that is used to automatically detect and register components 
  # for the usage on the rails view. default is nil, you can enable the feature by updating it in the next line.
  config.components_subdirectory = "ror_components"

  # components_subdirectory is the name of the subdirectory matched to detect and register components automatically
  # The default is nil. You can enable the feature by updating it in the next line.

Please update

Code quote:

  # for the usage on the rails view. default is nil, you can enable the feature by updating it in the next line.

docs/guides/file-system-based-automated-bundle-generation.md line 19 at r26 (raw file):

### Configure Components Subdirectory
`components_subdirectory`  is the name of the directories that contain components that will be automatically registered and used in Rails views.

`components_subdirectory`  is the name of the matched directories containing components that will be automatically registered for use by the view helpers.

Code quote:

`components_subdirectory`  is the name of the directories that contain components that will be automatically registered and used in Rails views.

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb line 51 at r26 (raw file):

  ################################################################################
  # components_subdirectory is the directory that is used to automatically detect and register components for the usage
  # on the rails view. default is nil, you can enable the feature by updating it in the next line.

`components_subdirectory`  is the name of the matching directories that contain automatically registered components for use in the Rails views.
The default is nil, you can enable the feature by updating it in the next line.

Code quote:

  # components_subdirectory is the directory that is used to automatically detect and register components for the usage
  # on the rails view. default is nil, you can enable the feature by updating it in the next line.

lib/react_on_rails/packs_generator.rb line 78 at r1 (raw file):

Previously, pulkitkkr (Pulkit) wrote…

@tomdracz Do you think it's to use FileUtils.rm_rf?

use parens

lib/react_on_rails/packs_generator.rb line 84 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@pulkitkkr

What if somebody used .ts, or.jsx or .tsx?

How about.

Get the file suffix

Substring without file suffix length, then append the -generated and the old suffix?

@pulkitkkr what about TypeScript???

lib/react_on_rails/packs_generator.rb line 105 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

no need for method

inline

@pulkitkkr ?

lib/react_on_rails/packs_generator.rb line 165 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

why have methods like this?

need to expose this for the api of packs_generator?

why not have some private methods and not have only static methods?

just have a factory method that creates an instance

@pulkitkkr

lib/react_on_rails/packs_generator.rb line 69 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I hate excluding parens for syntax like this.

yes, rubocop has no rule for this.

@pulkitkkr please change

lib/react_on_rails/packs_generator.rb line 113 at r25 (raw file):

Previously, pulkitkkr (Pulkit) wrote…

Done.

You need to throw an exception if you get a URL rather than a file. See how other callers of this method handle both files and URLs.

lib/react_on_rails/packs_generator.rb line 138 at r26 (raw file):

      component_file_pathname = Pathname.new ror_component_path
      component_generated_pack_path = generated_pack_path(ror_component_path)
      generated_pack_pathname = Pathname.new component_generated_pack_path

parens?

lib/react_on_rails/packs_generator.rb line 286 at r26 (raw file):

      file_content = File.read(file)

      return if file_content.include? text_to_prepend

parens

justin808

Reviewed 1 of 10 files at r26.
Reviewable status: all files reviewed, 52 unresolved discussions (waiting on @alexeyr, @alexeyr-ci1, @justin808, @pulkitkkr, and @tomdracz)

pulkitkkr

Reviewable status: all files reviewed, 51 unresolved discussions (waiting on @alexeyr, @alexeyr-ci1, @justin808, and @pulkitkkr)

docs/api/view-helpers-api.md line 30 at r26 (raw file):

Previously, justin808 (Justin Gordon) wrote…

- **auto_load_bundle:** will automatically load the bundle for component by calling `append_javascript_pack_tag` and `append_stylesheet_pack_tag` under the hood.

the bundle

Done.

docs/guides/configuration.md line 160 at r26 (raw file):

Previously, justin808 (Justin Gordon) wrote…

  # components_subdirectory is the name of the subdirectory matched to detect and register components automatically
  # The default is nil. You can enable the feature by updating it in the next line.

Please update

Done.

docs/guides/file-system-based-automated-bundle-generation.md line 19 at r26 (raw file):

Previously, justin808 (Justin Gordon) wrote…

`components_subdirectory`  is the name of the matched directories containing components that will be automatically registered for use by the view helpers.

Done.

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb line 51 at r26 (raw file):

Previously, justin808 (Justin Gordon) wrote…

`components_subdirectory`  is the name of the matching directories that contain automatically registered components for use in the Rails views.
The default is nil, you can enable the feature by updating it in the next line.

Done.

lib/react_on_rails/packs_generator.rb line 90 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I prefer parens consistently used except where idiomatic, like put 'abc', @alexeyr-ci1 @pulkitkkr

Done.

lib/react_on_rails/packs_generator.rb line 105 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@pulkitkkr ?

Done

lib/react_on_rails/packs_generator.rb line 165 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

why have methods like this?

need to expose this for the api of packs_generator?

why not have some private methods and not have only static methods?

just have a factory method that creates an instance

Done.

lib/react_on_rails/packs_generator.rb line 69 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@pulkitkkr please change

Done.

lib/react_on_rails/packs_generator.rb line 126 at r25 (raw file):

idiomatic
Updated to:

      component_file_pathname = Pathname.new ror_component_path
      component_generated_pack_path = generated_pack_path(ror_component_path)
      generated_pack_pathname = Pathname.new component_generated_pack_path

lib/react_on_rails/packs_generator.rb line 138 at r26 (raw file):

Previously, justin808 (Justin Gordon) wrote…

parens?

Done.

Code quote:

component_generated_pack_path

lib/react_on_rails/packs_generator.rb line 286 at r26 (raw file):

Previously, justin808 (Justin Gordon) wrote…

parens

Done.

lib/react_on_rails/test_helper/webpack_assets_status_checker.rb line 29 at r25 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I hate skipping the parens.

Done.

pulkitkkr

Reviewable status: 26 of 32 files reviewed, 51 unresolved discussions (waiting on @alexeyr, @alexeyr-ci1, @justin808, and @pulkitkkr)

lib/react_on_rails/packs_generator.rb line 84 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@pulkitkkr what about TypeScript???

Done.

pulkitkkr

Reviewable status: 25 of 32 files reviewed, 51 unresolved discussions (waiting on @alexeyr, @alexeyr-ci1, and @justin808)

docs/guides/file-system-based-automated-bundle-generation.md line 110 at r21 (raw file):

it's good to let folks know that are migrating. You can preface the explanatiion

lib/react_on_rails/packs_generator.rb line 165 at r5 (raw file):

Previously, pulkitkkr (Pulkit) wrote…

Done.

Done.

lib/react_on_rails/packs_generator.rb line 113 at r25 (raw file):

You need to throw an exception if you get a URL rather than a file. See how other callers of this method handle both files and URLs.

justin808

LGTM

Reviewed 4 of 6 files at r27, 3 of 3 files at r28, all commit messages.
Reviewable status: all files reviewed, 36 unresolved discussions (waiting on @alexeyr and @alexeyr-ci1)

dnesteryuk · 2023-02-03T14:21:32Z

Packs get generated after calling ReactOnRails::TestHelper.configure_rspec_to_compile_assets(config) in rails_helper.rb even though config.auto_load_bundle = false

react_on_rails/lib/react_on_rails/test_helper/ensure_assets_compiled.rb

Line 40 in 7b31394

ReactOnRails::PacksGenerator.instance.generate_packs_if_stale

In our case it breaks feature tests (an issue with sass variables, but there is no such an issue while running bin/webpacker), we don't need a pack for every component. I fixed our tests by adding:

module ReactOnRails
  class PacksGenerator
    def generate_packs_if_stale
      # does nothing
    end
  end
end

pulkitkkr mentioned this pull request May 30, 2022

WIP: Add File-System Based Components Pack Generation Functionality shakacode/shakapacker#116

Closed

pulkitkkr changed the title ~~Add Automated Files System Based Pack Generation Feature~~ WIP: Add Automated Files System Based Pack Generation Feature May 30, 2022

pulkitkkr force-pushed the pulkitkkr/automate-bundle-creation branch from 82ef861 to 1a72b37 Compare June 17, 2022 15:24

pulkitkkr commented Jun 17, 2022

View reviewed changes

lib/react_on_rails/packs_generator.rb Outdated Show resolved Hide resolved

pulkitkkr force-pushed the pulkitkkr/automate-bundle-creation branch from 14dc032 to 7e01dd4 Compare July 5, 2022 22:55

alexeyr-ci1 mentioned this pull request Jul 6, 2022

Remaining React 18 changes #1464

Closed

4 tasks

pulkitkkr force-pushed the pulkitkkr/automate-bundle-creation branch 3 times, most recently from d2cd1d4 to 31cca27 Compare July 7, 2022 23:56

alexeyr-ci1 reviewed Jul 8, 2022

View reviewed changes

lib/react_on_rails/helper.rb Outdated Show resolved Hide resolved

lib/react_on_rails/configuration.rb Outdated Show resolved Hide resolved

lib/react_on_rails/packs_generator.rb Outdated Show resolved Hide resolved

lib/react_on_rails/packs_generator.rb Outdated Show resolved Hide resolved

justin808 reviewed Jul 12, 2022

View reviewed changes

alexeyr-ci1 reviewed Jul 14, 2022

View reviewed changes

pulkitkkr force-pushed the pulkitkkr/automate-bundle-creation branch 2 times, most recently from b6ebf37 to d7f4acf Compare July 18, 2022 21:47

alexeyr-ci1 force-pushed the pulkitkkr/automate-bundle-creation branch from 1175779 to 752bafd Compare July 20, 2022 19:13

alexeyr reviewed Jul 20, 2022

View reviewed changes

lib/react_on_rails/packs_generator.rb Show resolved Hide resolved

alexeyr-ci1 force-pushed the pulkitkkr/automate-bundle-creation branch from 0287fa4 to 9feb90c Compare July 20, 2022 20:52

pulkitkkr force-pushed the pulkitkkr/automate-bundle-creation branch from 9272b19 to 59d5d8f Compare July 20, 2022 23:39

alexeyr-ci1 requested changes Jul 21, 2022

View reviewed changes

alexeyr-ci1 reviewed Jul 22, 2022

View reviewed changes

docs/guides/file-system-based-automated-bundle-generation.md Outdated Show resolved Hide resolved

alexeyr-ci1 reviewed Jul 22, 2022

View reviewed changes

docs/guides/file-system-based-automated-bundle-generation.md Outdated Show resolved Hide resolved

pulkitkkr added 7 commits August 16, 2022 22:49

Handle Shakapacker Requirements Unmet Issue

4fbe542

Address Grammar issues

14917db

Raise on nested_enteries false

8e2a9d8

Update example component names

d3574d8

Add Misc Error messages for Better Developer Experience

b0a92ed

Address Review Comments

31ad0b4

Address Docs related comments

656ec63

pulkitkkr force-pushed the pulkitkkr/automate-bundle-creation branch from 2a74be0 to 656ec63 Compare August 17, 2022 02:49

justin808 requested changes Aug 17, 2022

View reviewed changes

justin808 reviewed Aug 17, 2022

View reviewed changes

justin808 requested changes Aug 17, 2022

View reviewed changes

Rename components_directory to components_subdirectory

66a11bc

pulkitkkr commented Aug 17, 2022

View reviewed changes

pulkitkkr added 2 commits August 17, 2022 18:22

Address Comments

d6fb855

Remove static methods from Pack Generator

fd324b8

justin808 requested changes Aug 18, 2022

View reviewed changes

justin808 reviewed Aug 18, 2022

View reviewed changes

pulkitkkr commented Aug 18, 2022

View reviewed changes

pulkitkkr force-pushed the pulkitkkr/automate-bundle-creation branch 2 times, most recently from 3cbe059 to 6cc8895 Compare August 18, 2022 19:21

Address Review Comments

8c755d0

pulkitkkr force-pushed the pulkitkkr/automate-bundle-creation branch from 6cc8895 to 8c755d0 Compare August 18, 2022 19:21

Update Docs

31eea93

pulkitkkr commented Aug 18, 2022

View reviewed changes

justin808 approved these changes Aug 21, 2022

View reviewed changes

justin808 merged commit 9f4802d into master Aug 21, 2022

justin808 deleted the pulkitkkr/automate-bundle-creation branch August 21, 2022 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Automated Files System Based Pack Generation #1455

Add Automated Files System Based Pack Generation #1455

pulkitkkr commented May 30, 2022 •

edited

justin808 commented Jun 15, 2022

justin808 commented Jul 5, 2022

justin808 commented Jul 7, 2022

alexeyr-ci1 commented Jul 12, 2022

alexeyr-ci1 commented Jul 12, 2022

justin808 commented Jul 12, 2022

justin808 Jul 12, 2022 •

edited

alexeyr-ci1 Jul 14, 2022

alexeyr-ci1 Jul 14, 2022

alexeyr-ci1 commented Jul 14, 2022 •

edited

pulkitkkr commented Jul 21, 2022

pulkitkkr commented Aug 17, 2022

justin808 left a comment

justin808 left a comment

justin808 left a comment

pulkitkkr left a comment

justin808 left a comment

justin808 left a comment

pulkitkkr left a comment

pulkitkkr left a comment

pulkitkkr left a comment

justin808 left a comment

dnesteryuk commented Feb 3, 2023

Add Automated Files System Based Pack Generation #1455

Add Automated Files System Based Pack Generation #1455

Conversation

pulkitkkr commented May 30, 2022 • edited

Summary

Detail

justin808 commented Jun 15, 2022

justin808 commented Jul 5, 2022

justin808 commented Jul 7, 2022

alexeyr-ci1 commented Jul 12, 2022

alexeyr-ci1 commented Jul 12, 2022

justin808 commented Jul 12, 2022

justin808 Jul 12, 2022 • edited

Choose a reason for hiding this comment

default is nil

if defined:

alexeyr-ci1 Jul 14, 2022

Choose a reason for hiding this comment

alexeyr-ci1 Jul 14, 2022

Choose a reason for hiding this comment

alexeyr-ci1 commented Jul 14, 2022 • edited

pulkitkkr commented Jul 21, 2022

pulkitkkr commented Aug 17, 2022

justin808 left a comment

Choose a reason for hiding this comment

justin808 left a comment

Choose a reason for hiding this comment

justin808 left a comment

Choose a reason for hiding this comment

pulkitkkr left a comment

Choose a reason for hiding this comment

justin808 left a comment

Choose a reason for hiding this comment

justin808 left a comment

Choose a reason for hiding this comment

pulkitkkr left a comment

Choose a reason for hiding this comment

pulkitkkr left a comment

Choose a reason for hiding this comment

pulkitkkr left a comment

Choose a reason for hiding this comment

justin808 left a comment

Choose a reason for hiding this comment

dnesteryuk commented Feb 3, 2023

pulkitkkr commented May 30, 2022 •

edited

justin808 Jul 12, 2022 •

edited

alexeyr-ci1 commented Jul 14, 2022 •

edited