Add ReScript Example #1400

gscarv13 · 2021-11-03T21:42:51Z

This PR contains:

Add ReScript example component to the spec/dummy/
Boilerplate code provided by ljuster at React on Rails ReasonHelloWorld component #1215

This change is

Commit ljuster/react_on_rails@bedc479 Commit ljuster/react_on_rails@61c49ed

Create file to import compiled ReScript components

Add JavaScript dependency required to build for production

spec/dummy/.gitignore

justin808

Reviewed 11 of 12 files at r1, all commit messages.
Reviewable status: 11 of 12 files reviewed, 6 unresolved discussions (waiting on @gscarv13 and @justin808)

spec/dummy/package.json, line 63 at r1 (raw file):

    "build:clean": "rm -rf public/webpack || true",
    "re:build": "rescript build",
    "re:start": "rescript build -w"

remove these two right here
update build:XYZ with the rescript build
Add "build:production" and set ReactOnRails.configuration.build_production_command

spec/dummy/client/app/packs/client-bundle.js, line 14 at r1 (raw file):

import PureComponentWrappedInFunction from '../components/PureComponentWrappedInFunction';

import { HelloWorldReScript } from './rescript-components';

@pulkitkkr @alexfedoseev is this the right way to integrate?

spec/dummy/client/app/packs/rescript-components.js, line 8 at r1 (raw file):

export {
  HelloWorldReScript,

@pulkitkkr @alexfedoseev is this the right way to integrate?

spec/dummy/lib/tasks/before_assets_precompile.rake, line 3 at r1 (raw file):

# frozen_string_literal: true

task :before_assets_precompile do

Once you merge #1402 then we can remove this whole file.

spec/dummy/lib/tasks/before_assets_precompile.rake, line 5 at r1 (raw file):

task :before_assets_precompile do
  # clean and build rescript files
  system("yarn rescript clean && yarn rescript build")

Use sh

https://www.rubydoc.info/gems/rake/FileUtils:sh
https://newbedev.com/how-to-execute-commands-within-rake-tasks

throw if command does not run cleanly.

and don't put script here, put script in package.json

`yarn build:

justin808

Need to add test to https://github.com/shakacode/react_on_rails/blob/master/spec/dummy/spec/system/integration_spec.rb

Reviewed 1 of 12 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @gscarv13)

add files to .prettierignore

spec/dummy/.gitignore

spec/dummy/.prettierignore

spec/dummy/bsconfig.json

spec/dummy/client/app/packs/rescript-components.js

spec/dummy/package.json

spec/dummy/spec/system/integration_spec.rb

Remove babel-plugin-transform-react-remove-prop-types

justin808

Reviewed 3 of 6 files at r2, 2 of 3 files at r5, 1 of 3 files at r6, 6 of 6 files at r7, all commit messages.
Reviewable status: 16 of 17 files reviewed, 13 unresolved discussions (waiting on @alexfedoseev, @gscarv13, and @justin808)

spec/dummy/.gitignore, line 27 at r5 (raw file):

Previously, gscarv13 (Gustavo Carvalho) wrote…

@justin808 The ReScript compiler put the bs/ inside lib/. Looking at the documentation I couldn't find a way to change it

Hey @alexfedoseev it's very common to have a /lib directory as part of Rails apps.

@gscarv13 Can ReScript be configured to have the folder called /rescript-lib ?

spec/dummy/package.json, line 14 at r5 (raw file):

Previously, gscarv13 (Gustavo Carvalho) wrote…

I was building the app for the tests and that was being required. I'll check if it is still relevant @justin808

this is for production builds to remove prop types

so yes, good

and not required in this PR.

separate PR would be good, definitely the update to rails/webpacker

spec/dummy/client/app/components/HelloWorldReScript.res, line 4 at r7 (raw file):

// is the same hash passed to react_component method on the view. Log the Props in the compiled JS
// and make sure the props passed to the component below match the shape of the prop passed on the view.

Comment on why named default

spec/dummy/client/app/packs/rescript-components.js, line 5 at r7 (raw file):

// and they are generated on the same directory as the .res file

import HelloWorldReScript from '../components/HelloWorldReScript.bs';

@alexfedoseev should we import the bs file from the JS file like this?

or maybe

import HelloWorldReScript from '../components/HelloWorldReScript.bs.js';

or configure webpack/babel so this works:

import HelloWorldReScript from '../components/HelloWorldReScript';

and there is a default to look for the .bs.js file?

spec/dummy/spec/system/integration_spec.rb, line 123 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

test changing the text in the input field and observing the output

OK

mightystrong

Reviewed 3 of 12 files at r1, 2 of 6 files at r2, 2 of 3 files at r5, 3 of 6 files at r7, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @alexfedoseev and @gscarv13)

spec/dummy/.gitignore, line 27 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Hey @alexfedoseev it's very common to have a /lib directory as part of Rails apps.

@gscarv13 Can ReScript be configured to have the folder called /rescript-lib ?

I agree that the lib directory is an odd placement.

spec/dummy/bsconfig.json, line 13 at r5 (raw file):

Previously, alexfedoseev (Alex Fedoseev) wrote…

It's not relevant anymore.

@alexfedoseev Should we remove it from here too? https://github.com/shakacode/rescript-on-rails-example/blob/main/bsconfig.json

spec/dummy/app/views/pages/client_side_rescript_hello_world.html.erb, line 4 at r7 (raw file):

<hr/>

<h1>React Rails Client Side Only Rendering</h1>

Seems odd to me to mix Rails HTML in a specific view instead of delegating all the front end in the view to ReScript, unless there's a compelling reason. Devs have to bounce between the Rails view and ReScript to decide where to edit the HTML. In my opinion it's cleaner to separate concerns.

spec/dummy/client/app/packs/rescript-components.js, line 5 at r7 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@alexfedoseev should we import the bs file from the JS file like this?

or maybe
import HelloWorldReScript from '../components/HelloWorldReScript.bs.js';
or configure webpack/babel so this works:
import HelloWorldReScript from '../components/HelloWorldReScript';
and there is a default to look for the .bs.js file?

@justin808 It would be nice if the .bs.js was implied.

mightystrong

Reviewed 1 of 12 files at r1, 2 of 6 files at r2, 3 of 6 files at r7.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @alexfedoseev and @gscarv13)

mightystrong

Reviewed 1 of 3 files at r6.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @alexfedoseev and @gscarv13)

gscarv13

Reviewable status: 9 of 17 files reviewed, 14 unresolved discussions (waiting on @alexfedoseev, @gscarv13, @justin808, and @mightystrong)

spec/dummy/.gitignore, line 29 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Please note these chars

Done.

spec/dummy/.gitignore, line 27 at r5 (raw file):

Previously, mightystrong (Michael Price) wrote…

I agree that the lib directory is an odd placement.

Sadly there's no way of configuring it in rescript yet rescript-lang/rescript#1124 (comment)

spec/dummy/bsconfig.json, line 13 at r5 (raw file):

Previously, mightystrong (Michael Price) wrote…

@alexfedoseev Should we remove it from here too? https://github.com/shakacode/rescript-on-rails-example/blob/main/bsconfig.json

Done.

spec/dummy/bsconfig.json, line 16 at r5 (raw file):

Previously, alexfedoseev (Alex Fedoseev) wrote…

Useless in the context of the apps.

Done.

spec/dummy/package.json, line 62 at r5 (raw file):

Previously, alexfedoseev (Alex Fedoseev) wrote…

You probably want -with-deps flag here.

Done.

spec/dummy/app/views/pages/client_side_rescript_hello_world.html.erb, line 4 at r7 (raw file):

Previously, mightystrong (Michael Price) wrote…

Seems odd to me to mix Rails HTML in a specific view instead of delegating all the front end in the view to ReScript, unless there's a compelling reason. Devs have to bounce between the Rails view and ReScript to decide where to edit the HTML. In my opinion it's cleaner to separate concerns.

Since this is already used in another view, I'll remove it from this one

spec/dummy/client/app/components/HelloWorldReScript.res, line 4 at r7 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Comment on why named default

Done.

spec/dummy/client/app/packs/rescript-components.js, line 5 at r7 (raw file):

Previously, mightystrong (Michael Price) wrote…

@justin808 It would be nice if the .bs.js was implied.

I find it better to be explicit and import as .bs.js'; to make clear it's a rescript component

spec/dummy/lib/tasks/before_assets_precompile.rake, line 3 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Once you merge #1402 then we can remove this whole file.

Done.

spec/dummy/lib/tasks/before_assets_precompile.rake, line 5 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Use sh

https://www.rubydoc.info/gems/rake/FileUtils:sh
https://newbedev.com/how-to-execute-commands-within-rake-tasks

throw if command does not run cleanly.

and don't put script here, put script in package.json

`yarn build:

Done.

gscarv13

Reviewable status: 9 of 17 files reviewed, 14 unresolved discussions (waiting on @alexfedoseev, @gscarv13, @justin808, and @mightystrong)

spec/dummy/package.json, line 14 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

this is for production builds to remove prop types

so yes, good

and not required in this PR.

separate PR would be good, definitely the update to rails/webpacker

Done.

justin808

Looks ready to merge. So long as CI passes, we can merge.

Reviewed 2 of 4 files at r8, 6 of 6 files at r9, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @alexfedoseev, @gscarv13, and @mightystrong)

spec/dummy/client/app/packs/rescript-components.js, line 5 at r7 (raw file):

Previously, gscarv13 (Gustavo Carvalho) wrote…

I find it better to be explicit and import as .bs.js'; to make clear it's a rescript component

OK

gscarv13 added 4 commits November 3, 2021 13:57

Cherry-pick commits from shakacode#1215

51cc84f

Commit ljuster/react_on_rails@bedc479 Commit ljuster/react_on_rails@61c49ed

Add ReScript dependencies and configuration

6f8aae2

Remove Reason file, Replace Reason occurrences by ReScript

fb7959c

Add ReScript component, update yarn.lock at spec/dummy/

49a45da

Create file to import compiled ReScript components

gscarv13 mentioned this pull request Nov 3, 2021

Update the install generator to add Webpacker configuration, Add ReScript component and Fix Issue #1397 #1399

Closed

Add rake task to build ReScript when running rake assets:precompile

46db32d

Add JavaScript dependency required to build for production

justin808 reviewed Nov 3, 2021

View reviewed changes

spec/dummy/.gitignore Outdated Show resolved Hide resolved

justin808 requested changes Nov 3, 2021

View reviewed changes

justin808 reviewed Nov 3, 2021

View reviewed changes

gscarv13 added 7 commits November 4, 2021 13:28

Fix .gitignore

54f8053

Update build scripts on spec/dummy, Remove Rake task to run ReScript

c962371

Add request unit test for rescript component

7f570e1

Add config.build_production_command to dummy

e06a5a8

Update dummy scripts

5ca7e27

Run prettier and add *.bs.js files to .prettierignore

b385901

Add return at the end of the line package.json

cf41cc3

gscarv13 force-pushed the add-rescript-demo branch 3 times, most recently from f0d1922 to 15baa04 Compare November 5, 2021 00:44

Bump prettier, concurrently and prettier-eslint-cli

b6179b3

add files to .prettierignore

gscarv13 force-pushed the add-rescript-demo branch from 15baa04 to b6179b3 Compare November 5, 2021 00:48

alex35mil reviewed Nov 5, 2021

View reviewed changes

justin808 requested changes Nov 5, 2021

View reviewed changes

spec/dummy/package.json Outdated Show resolved Hide resolved

spec/dummy/spec/system/integration_spec.rb Outdated Show resolved Hide resolved

gscarv13 added 2 commits November 5, 2021 10:36

Update bsconfig.json

21a9f54

Update ReScript component import

78f9a46

gscarv13 force-pushed the add-rescript-demo branch 2 times, most recently from 5c87c07 to 3d367b7 Compare November 5, 2021 15:55

gscarv13 added 2 commits November 5, 2021 13:44

Fix prettier issue, Remove dependency from package.json

11377e1

Remove babel-plugin-transform-react-remove-prop-types

Update ReScript test, Update comment on ReScript component

f471848

gscarv13 force-pushed the add-rescript-demo branch from 3d367b7 to f471848 Compare November 5, 2021 16:51

gscarv13 force-pushed the add-rescript-demo branch from 68acea5 to 95f8b26 Compare November 6, 2021 15:04

Fix new line at the end of file, Fix dummy package.json scripts

f69e21a

gscarv13 force-pushed the add-rescript-demo branch from 95f8b26 to f69e21a Compare November 6, 2021 15:27

justin808 requested changes Nov 19, 2021

View reviewed changes

mightystrong reviewed Dec 10, 2021

View reviewed changes

gscarv13 added 2 commits December 29, 2021 11:28

Merge shakacode:master

66143b6

Address requested changes

0e87058

gscarv13 commented Dec 29, 2021

View reviewed changes

justin808 reviewed Dec 29, 2021

View reviewed changes

justin808 merged commit dad754c into shakacode:master Dec 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ReScript Example #1400

Add ReScript Example #1400

gscarv13 commented Nov 3, 2021 •

edited by justin808

Loading

justin808 left a comment

justin808 left a comment

justin808 left a comment

mightystrong left a comment

mightystrong left a comment

mightystrong left a comment

gscarv13 left a comment

gscarv13 left a comment

justin808 left a comment

Add ReScript Example #1400

Add ReScript Example #1400

Conversation

gscarv13 commented Nov 3, 2021 • edited by justin808 Loading

This PR contains:

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

mightystrong left a comment

Choose a reason for hiding this comment

mightystrong left a comment

Choose a reason for hiding this comment

mightystrong left a comment

Choose a reason for hiding this comment

gscarv13 left a comment

Choose a reason for hiding this comment

gscarv13 left a comment

Choose a reason for hiding this comment

justin808 left a comment

Choose a reason for hiding this comment

gscarv13 commented Nov 3, 2021 •

edited by justin808

Loading