Skip to content

Conversation

@seuros
Copy link
Contributor

@seuros seuros commented Aug 8, 2016

can you please check this PR @forelabs ?

@forelabs
Copy link

forelabs commented Aug 8, 2016

  • no comment in compiled asset
  • comment converted into required component code

works like a charm!

@rmosolgo
Copy link
Member

rmosolgo commented Aug 9, 2016

@seuros what do you think about adding a test case for this?

I'm trying to see if I understand ... since we're defining a new mime type in react-rails, we also have to tell Sprockets to preprocess //= requires in this new mime type, right? It seems like we should add a test case to cover //= requires in the new mime type.

Maybe you could add a new directory, assets/javascripts/require_test/, and make a few files, then add a .jsx file which requires them, eg

// app/assets/javascripts/require_test/jsx_require_test.jsx
//= require ./jsx_require_child_1
//= require ./jsx_require_child_2

<div className="own-div" />

Then, fetch the "compiled" asset and make sure it includes all required content:

jsx_require_asset = Rails.application.assets["require_test/jsx_require_test"].to_s 
# Make sure the asset has required content & own content:
assert_includes jsx_require_asset, jsx_require_child_1_content
assert_includes jsx_require_asset, jsx_require_child_w_content
assert_includes jsx_require_asset, own_content 

Or, another way to make sure that this works the way we expect in the future?

@seuros
Copy link
Contributor Author

seuros commented Aug 9, 2016

@rmosolgo Sure thing.

React.createElement(\"div\", { className: \"le-javascript\" });
javascript

test 'accepts harmony: true option' do
Copy link
Member

Choose a reason for hiding this comment

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

should be test "executes //= require directives" do ?

@seuros seuros force-pushed the patch-1 branch 2 times, most recently from 3c0e15b to 05453a2 Compare August 9, 2016 18:19
@seuros
Copy link
Contributor Author

seuros commented Aug 9, 2016

@rmosolgo we can remove the work in progress in sprocket 4 now.

@rmosolgo
Copy link
Member

rmosolgo commented Aug 9, 2016

🎉 thanks, i'll merge and ship 1.8.2 pronto.

@rmosolgo rmosolgo merged commit 004d3d9 into reactjs:master Aug 9, 2016
@seuros seuros deleted the patch-1 branch August 9, 2016 19:01
@rmosolgo
Copy link
Member

rmosolgo commented Aug 9, 2016

removed the sprockets 4 note in 5049473

@rmosolgo
Copy link
Member

rmosolgo commented Aug 9, 2016

🚢 1.8.2 🎊 thanks again!

@rmosolgo rmosolgo mentioned this pull request Aug 12, 2016
4 tasks
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