Support singleton webpacker (3 and above) - #777

Merged
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776
Sep 18, 2017
Merged

Support singleton webpacker (3 and above)#777
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776

Conversation

@BookOfGreg

@BookOfGregBookOfGreg commented Aug 31, 2017

Copy link
Copy Markdown
Contributor

Fixes#776
Fixes#775
Fixes#778
Fixes#780
Fixes#781
Fixes#782

Once again I could probably use some assistance with the appraisal gem but other than that should be OK.

Edit: This turned into a BIG refactor of the tests to split out support and I'll be honest my code isn't the tidiest. Still plenty of refactoring to go as I'm not sure I'm thrilled about how I've switched method definitions of Webpacker version numbers.

private

def webpack_configuration
Webpacker.respond_to?(:config) ? Webpacker.config : webpack_configuration

@AnatoliiDAnatoliiDSep 1, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should be

Webpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configuration

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Aaw whoops. Wondered why my test didn't pass XD Thanks

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've still got issues around Webpacker::Manifest.load, seems to be how I detect respond_to? may be wrong for that one.

@bradleesandbradleesand left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The install generator won't work because entry_path was removed from webpacker config.

Webpacker::Configuration.source_path
.join(Webpacker::Configuration.entry_path)
webpack_configuration.source_path
.join(webpack_configuration.entry_path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new singleton implementation of Webpacker::Configuration no longer has entry_path. Instead, Webpacker::Configuration.source_path.join(Webpacker::Configuration.entry_path) becomes Webpacker.config.source_entry_path

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have suggested these changes in issue #778 .Please have a look into it. I was trying to install react with rails and failed to run react installer command. I look into gem and found that method calling is not proper.

I fix this with minor changes in lib/generators/react/install_generator.rb file.

source_path is a instance method and was being called by class directly. Also entry_path was changed to source_entry_path in webpacker config. I have suggested these changes in #778

The main purpose for issue was #778 was to elevate exact issue and its solution along with it, as there was no fix until that time.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using webpacker 3.0.1 Webpacker.config.source_entry_path worked for me 👍

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks good, only a minor change suggested that webpack_configuration.entry_path should be changed to webpack_configuration.source_entry_path, as entry_path is renamed as source_entry_path in webpacker config file. File location for this file is lib/generators/react/install_generator.rb under method name javascript_dir.

@BookOfGreg

BookOfGreg commented Sep 5, 2017

Copy link
Copy Markdown
ContributorAuthor

I took on the code from #778 , thank you for that one! Makes it a little easier to update.

Currently working on trying to make the new config/webpacker.yml and it's env files play nicely with webpacker V1's paths.yml. It's putting my manifest in the webpacker.yml's output location with the paths.yml's manifest name so the webpacker helpers can't find the manifest.

Will take me some time to figure out all the subtleties of this before I'm done. Very welcome for someone to help with this!

Edit: Is there a nice way of changing the config files in test/dummy?

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In file lib/react/server_rendering/webpacker_manifest_container.rb, I am looking at a method name lookup_path at line no. 24, but I don't think it is defined anywhere in the both react-rails & webpacker gem. This will through an error once find_asset(logical_path) method is being called.

Please look into it! Are you guys loading any file with such variable?

Apart for this, hello @BookOfGreg, what kind of changes you wanna in test/dummy config files. Does it contains any Json data? Please share, hopefully I may change that!

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Webpacker 1 and 2 have lookup_path, Webpacker 3 does not. and I'll have to compose a method of finding the actual path of a file.

@basicBrogrammer

Copy link
Copy Markdown

I'm not sure if this is related, but the component generator is putting my components in app/javascript/packs/components. To get this to work, I had to change the require context from 'components' to './components'

app/javascript/packs/application.js
var componentRequireContext = require.context('./components', true')

And also in my es6 components I have to require('../components/NameOfComponent')
Any thoughts?

var NewList = function() { return <span>"New List"</span> }
HEREDOC
new_file_contents = <<-HEREDOC
var NewList = function() { return <span>"New List"</span> }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like you might've messed up the indentation here

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The <<- syntax allows the closing HEREDOC to be indented but it won't strip space from the content so this works just fine.

end

def config
!!defined?(Webpacker::Configuration) ? Webpacker::Configuration : Webpacker.config

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this needs to be

defconfigWebpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configurationend

Because even in Webpacker 3, the Webpacker::Configuration constant is defined.

@tomwaddington

Copy link
Copy Markdown

Just ran into issue #778, and this fixed it for me. 👍

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

I'll admit that I'm at a bit of a dead end. On #779 I've got all of Travis passing except Rails 5 + Webpacker, but on that branch Webpack builds to public/packs but Webpacker looks in packs/packs instead. That is the case even if I peg Webpack at 1.15.x and Webpacker at ~> 1.0.
If a regular committer could help me out to get the tests passing on master then I could continue working here to get more Webpacker support.

We should really think about the maintainability of the Webpacker support as the manifest paths between versions seems really flakey and i'm not keen on the pattern of .responds_to? and defined? everywhere.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Master finally passes and this does mean Webpacker 3 support (And 1.1, 1.2, 2.0) on this branch.
I split out as much as I could do make it clearer but there is still a lot of mess I caused in the code, lots of refactoring to do.

@BookOfGregBookOfGreg mentioned this pull request Sep 12, 2017
6 tasks
@@ -1,4 +1,5 @@
require "open-uri"
# require 'pry'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rmosolgo

Copy link
Copy Markdown
Contributor

Wow, this is really impressive! Thanks for all your work in digging in on this. It's nice to see that Webpacker has provided some APIs to support this kind of thing, too.

I see the green check, and the tests look good. So, what's still to go on this one?

@ArnonHongklay

Copy link
Copy Markdown

👍

@BookOfGreg

BookOfGreg commented Sep 13, 2017

Copy link
Copy Markdown
ContributorAuthor

This could go in to master it's current state as I believe it to be an improvement but frankly this could use some TLC and refactoring.

I'm certain all my MAJOR < 3 checks could be extracted into a class following the existing pattern.
There are probably better ways of integrating with the Webpacker.dev_server also.

I found some odd behaviour such as webpack 3 seems to read the wrong node_modules folder when loading in ../../../../react_ujs, so I used the actual npm package for it instead, that might cause some people in the wild issues that I can't even predict if they're messing with node context.

All these tweaks could be in future patches to this though.

Edit: Since I've modified the webpacker 1.1, 1.2 and 2 support, it would be good to see if it still works without bugs on an existing project for confirmation.

@Melonbwead

Copy link
Copy Markdown

You beast @BookOfGreg 👍 ❗️

@BookOfGreg

BookOfGreg commented Sep 15, 2017

Copy link
Copy Markdown
ContributorAuthor

I think we should probably merge this if we can be confident that it works. All existing tests pass. I can carry on with a tidy up beyond this.

Edit: apparently linting broke the tests. How odd. Will keep looking into it.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Tests fail on specific seeds:
bundle exec appraisal rails-5_no_sprockets_webpacker_3 rake test TESTOPTS="--seed=17880"

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

@rmosolgo If you like, :shipit: and I'll keep PR'ing rubocops, test fixes and refactorings as patch-level changes after this. Be good to start getting some bug reports from folks so I know if this accidentally impacts anyone.

@rmosolgo

Copy link
Copy Markdown
Contributor

That sounds great, let me go ahead with a release!

@rmosolgo
rmosolgo merged commit 901f58d into reactjs:masterSep 18, 2017
@gaiapunk

gaiapunk commented Sep 20, 2017

Copy link
Copy Markdown

I'm still having the #778 issue and was wondering if this latest merge had been release to fix it or if there is a workaround on my end that I can do before a fix is released, thanks for all the work you do!

@BookOfGreg

BookOfGreg commented Sep 20, 2017

Copy link
Copy Markdown
ContributorAuthor

@gaiapunk It's not been released as a gem yet but it's possible to use the master branch directly with:
gem 'react-rails', git: 'https://github.com/reactjs/react-rails.git', branch: 'master'

@BookOfGreg
BookOfGreg deleted the fix-776 branch September 20, 2017 09:19
@BookOfGregBookOfGreg mentioned this pull request Sep 27, 2017
Sign up for freeto 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.

13 participants

@BookOfGreg@basicBrogrammer@tomwaddington@rmosolgo@ArnonHongklay@Melonbwead@gaiapunk@willcosgrove@swrobel@AnatoliiD@bradleesand@RiccardoMargiotta@vikash-k-singh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Support singleton webpacker (3 and above) - #777

Merged
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776
Sep 18, 2017
Merged

Support singleton webpacker (3 and above)#777
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776

Conversation

@BookOfGreg

@BookOfGregBookOfGreg commented Aug 31, 2017

Copy link
Copy Markdown
Contributor

Fixes#776
Fixes#775
Fixes#778
Fixes#780
Fixes#781
Fixes#782

Once again I could probably use some assistance with the appraisal gem but other than that should be OK.

Edit: This turned into a BIG refactor of the tests to split out support and I'll be honest my code isn't the tidiest. Still plenty of refactoring to go as I'm not sure I'm thrilled about how I've switched method definitions of Webpacker version numbers.

private

def webpack_configuration
Webpacker.respond_to?(:config) ? Webpacker.config : webpack_configuration

@AnatoliiDAnatoliiDSep 1, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should be

Webpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configuration

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Aaw whoops. Wondered why my test didn't pass XD Thanks

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've still got issues around Webpacker::Manifest.load, seems to be how I detect respond_to? may be wrong for that one.

@bradleesandbradleesand left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The install generator won't work because entry_path was removed from webpacker config.

Webpacker::Configuration.source_path
.join(Webpacker::Configuration.entry_path)
webpack_configuration.source_path
.join(webpack_configuration.entry_path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new singleton implementation of Webpacker::Configuration no longer has entry_path. Instead, Webpacker::Configuration.source_path.join(Webpacker::Configuration.entry_path) becomes Webpacker.config.source_entry_path

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have suggested these changes in issue #778 .Please have a look into it. I was trying to install react with rails and failed to run react installer command. I look into gem and found that method calling is not proper.

I fix this with minor changes in lib/generators/react/install_generator.rb file.

source_path is a instance method and was being called by class directly. Also entry_path was changed to source_entry_path in webpacker config. I have suggested these changes in #778

The main purpose for issue was #778 was to elevate exact issue and its solution along with it, as there was no fix until that time.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using webpacker 3.0.1 Webpacker.config.source_entry_path worked for me 👍

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks good, only a minor change suggested that webpack_configuration.entry_path should be changed to webpack_configuration.source_entry_path, as entry_path is renamed as source_entry_path in webpacker config file. File location for this file is lib/generators/react/install_generator.rb under method name javascript_dir.

@BookOfGreg

BookOfGreg commented Sep 5, 2017

Copy link
Copy Markdown
ContributorAuthor

I took on the code from #778 , thank you for that one! Makes it a little easier to update.

Currently working on trying to make the new config/webpacker.yml and it's env files play nicely with webpacker V1's paths.yml. It's putting my manifest in the webpacker.yml's output location with the paths.yml's manifest name so the webpacker helpers can't find the manifest.

Will take me some time to figure out all the subtleties of this before I'm done. Very welcome for someone to help with this!

Edit: Is there a nice way of changing the config files in test/dummy?

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In file lib/react/server_rendering/webpacker_manifest_container.rb, I am looking at a method name lookup_path at line no. 24, but I don't think it is defined anywhere in the both react-rails & webpacker gem. This will through an error once find_asset(logical_path) method is being called.

Please look into it! Are you guys loading any file with such variable?

Apart for this, hello @BookOfGreg, what kind of changes you wanna in test/dummy config files. Does it contains any Json data? Please share, hopefully I may change that!

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Webpacker 1 and 2 have lookup_path, Webpacker 3 does not. and I'll have to compose a method of finding the actual path of a file.

@basicBrogrammer

Copy link
Copy Markdown

I'm not sure if this is related, but the component generator is putting my components in app/javascript/packs/components. To get this to work, I had to change the require context from 'components' to './components'

app/javascript/packs/application.js
var componentRequireContext = require.context('./components', true')

And also in my es6 components I have to require('../components/NameOfComponent')
Any thoughts?

var NewList = function() { return <span>"New List"</span> }
HEREDOC
new_file_contents = <<-HEREDOC
var NewList = function() { return <span>"New List"</span> }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like you might've messed up the indentation here

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The <<- syntax allows the closing HEREDOC to be indented but it won't strip space from the content so this works just fine.

end

def config
!!defined?(Webpacker::Configuration) ? Webpacker::Configuration : Webpacker.config

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this needs to be

defconfigWebpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configurationend

Because even in Webpacker 3, the Webpacker::Configuration constant is defined.

@tomwaddington

Copy link
Copy Markdown

Just ran into issue #778, and this fixed it for me. 👍

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

I'll admit that I'm at a bit of a dead end. On #779 I've got all of Travis passing except Rails 5 + Webpacker, but on that branch Webpack builds to public/packs but Webpacker looks in packs/packs instead. That is the case even if I peg Webpack at 1.15.x and Webpacker at ~> 1.0.
If a regular committer could help me out to get the tests passing on master then I could continue working here to get more Webpacker support.

We should really think about the maintainability of the Webpacker support as the manifest paths between versions seems really flakey and i'm not keen on the pattern of .responds_to? and defined? everywhere.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Master finally passes and this does mean Webpacker 3 support (And 1.1, 1.2, 2.0) on this branch.
I split out as much as I could do make it clearer but there is still a lot of mess I caused in the code, lots of refactoring to do.

@BookOfGregBookOfGreg mentioned this pull request Sep 12, 2017
6 tasks
@@ -1,4 +1,5 @@
require "open-uri"
# require 'pry'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rmosolgo

Copy link
Copy Markdown
Contributor

Wow, this is really impressive! Thanks for all your work in digging in on this. It's nice to see that Webpacker has provided some APIs to support this kind of thing, too.

I see the green check, and the tests look good. So, what's still to go on this one?

@ArnonHongklay

Copy link
Copy Markdown

👍

@BookOfGreg

BookOfGreg commented Sep 13, 2017

Copy link
Copy Markdown
ContributorAuthor

This could go in to master it's current state as I believe it to be an improvement but frankly this could use some TLC and refactoring.

I'm certain all my MAJOR < 3 checks could be extracted into a class following the existing pattern.
There are probably better ways of integrating with the Webpacker.dev_server also.

I found some odd behaviour such as webpack 3 seems to read the wrong node_modules folder when loading in ../../../../react_ujs, so I used the actual npm package for it instead, that might cause some people in the wild issues that I can't even predict if they're messing with node context.

All these tweaks could be in future patches to this though.

Edit: Since I've modified the webpacker 1.1, 1.2 and 2 support, it would be good to see if it still works without bugs on an existing project for confirmation.

@Melonbwead

Copy link
Copy Markdown

You beast @BookOfGreg 👍 ❗️

@BookOfGreg

BookOfGreg commented Sep 15, 2017

Copy link
Copy Markdown
ContributorAuthor

I think we should probably merge this if we can be confident that it works. All existing tests pass. I can carry on with a tidy up beyond this.

Edit: apparently linting broke the tests. How odd. Will keep looking into it.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Tests fail on specific seeds:
bundle exec appraisal rails-5_no_sprockets_webpacker_3 rake test TESTOPTS="--seed=17880"

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

@rmosolgo If you like, :shipit: and I'll keep PR'ing rubocops, test fixes and refactorings as patch-level changes after this. Be good to start getting some bug reports from folks so I know if this accidentally impacts anyone.

@rmosolgo

Copy link
Copy Markdown
Contributor

That sounds great, let me go ahead with a release!

@rmosolgo
rmosolgo merged commit 901f58d into reactjs:masterSep 18, 2017
@gaiapunk

gaiapunk commented Sep 20, 2017

Copy link
Copy Markdown

I'm still having the #778 issue and was wondering if this latest merge had been release to fix it or if there is a workaround on my end that I can do before a fix is released, thanks for all the work you do!

@BookOfGreg

BookOfGreg commented Sep 20, 2017

Copy link
Copy Markdown
ContributorAuthor

@gaiapunk It's not been released as a gem yet but it's possible to use the master branch directly with:
gem 'react-rails', git: 'https://github.com/reactjs/react-rails.git', branch: 'master'

@BookOfGreg
BookOfGreg deleted the fix-776 branch September 20, 2017 09:19
@BookOfGregBookOfGreg mentioned this pull request Sep 27, 2017
Sign up for freeto 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.

13 participants

@BookOfGreg@basicBrogrammer@tomwaddington@rmosolgo@ArnonHongklay@Melonbwead@gaiapunk@willcosgrove@swrobel@AnatoliiD@bradleesand@RiccardoMargiotta@vikash-k-singh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Support singleton webpacker (3 and above) - #777

Merged
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776
Sep 18, 2017
Merged

Support singleton webpacker (3 and above)#777
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776

Conversation

@BookOfGreg

@BookOfGregBookOfGreg commented Aug 31, 2017

Copy link
Copy Markdown
Contributor

Fixes#776
Fixes#775
Fixes#778
Fixes#780
Fixes#781
Fixes#782

Once again I could probably use some assistance with the appraisal gem but other than that should be OK.

Edit: This turned into a BIG refactor of the tests to split out support and I'll be honest my code isn't the tidiest. Still plenty of refactoring to go as I'm not sure I'm thrilled about how I've switched method definitions of Webpacker version numbers.

private

def webpack_configuration
Webpacker.respond_to?(:config) ? Webpacker.config : webpack_configuration

@AnatoliiDAnatoliiDSep 1, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should be

Webpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configuration

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Aaw whoops. Wondered why my test didn't pass XD Thanks

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've still got issues around Webpacker::Manifest.load, seems to be how I detect respond_to? may be wrong for that one.

@bradleesandbradleesand left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The install generator won't work because entry_path was removed from webpacker config.

Webpacker::Configuration.source_path
.join(Webpacker::Configuration.entry_path)
webpack_configuration.source_path
.join(webpack_configuration.entry_path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new singleton implementation of Webpacker::Configuration no longer has entry_path. Instead, Webpacker::Configuration.source_path.join(Webpacker::Configuration.entry_path) becomes Webpacker.config.source_entry_path

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have suggested these changes in issue #778 .Please have a look into it. I was trying to install react with rails and failed to run react installer command. I look into gem and found that method calling is not proper.

I fix this with minor changes in lib/generators/react/install_generator.rb file.

source_path is a instance method and was being called by class directly. Also entry_path was changed to source_entry_path in webpacker config. I have suggested these changes in #778

The main purpose for issue was #778 was to elevate exact issue and its solution along with it, as there was no fix until that time.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using webpacker 3.0.1 Webpacker.config.source_entry_path worked for me 👍

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks good, only a minor change suggested that webpack_configuration.entry_path should be changed to webpack_configuration.source_entry_path, as entry_path is renamed as source_entry_path in webpacker config file. File location for this file is lib/generators/react/install_generator.rb under method name javascript_dir.

@BookOfGreg

BookOfGreg commented Sep 5, 2017

Copy link
Copy Markdown
ContributorAuthor

I took on the code from #778 , thank you for that one! Makes it a little easier to update.

Currently working on trying to make the new config/webpacker.yml and it's env files play nicely with webpacker V1's paths.yml. It's putting my manifest in the webpacker.yml's output location with the paths.yml's manifest name so the webpacker helpers can't find the manifest.

Will take me some time to figure out all the subtleties of this before I'm done. Very welcome for someone to help with this!

Edit: Is there a nice way of changing the config files in test/dummy?

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In file lib/react/server_rendering/webpacker_manifest_container.rb, I am looking at a method name lookup_path at line no. 24, but I don't think it is defined anywhere in the both react-rails & webpacker gem. This will through an error once find_asset(logical_path) method is being called.

Please look into it! Are you guys loading any file with such variable?

Apart for this, hello @BookOfGreg, what kind of changes you wanna in test/dummy config files. Does it contains any Json data? Please share, hopefully I may change that!

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Webpacker 1 and 2 have lookup_path, Webpacker 3 does not. and I'll have to compose a method of finding the actual path of a file.

@basicBrogrammer

Copy link
Copy Markdown

I'm not sure if this is related, but the component generator is putting my components in app/javascript/packs/components. To get this to work, I had to change the require context from 'components' to './components'

app/javascript/packs/application.js
var componentRequireContext = require.context('./components', true')

And also in my es6 components I have to require('../components/NameOfComponent')
Any thoughts?

var NewList = function() { return <span>"New List"</span> }
HEREDOC
new_file_contents = <<-HEREDOC
var NewList = function() { return <span>"New List"</span> }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like you might've messed up the indentation here

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The <<- syntax allows the closing HEREDOC to be indented but it won't strip space from the content so this works just fine.

end

def config
!!defined?(Webpacker::Configuration) ? Webpacker::Configuration : Webpacker.config

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this needs to be

defconfigWebpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configurationend

Because even in Webpacker 3, the Webpacker::Configuration constant is defined.

@tomwaddington

Copy link
Copy Markdown

Just ran into issue #778, and this fixed it for me. 👍

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

I'll admit that I'm at a bit of a dead end. On #779 I've got all of Travis passing except Rails 5 + Webpacker, but on that branch Webpack builds to public/packs but Webpacker looks in packs/packs instead. That is the case even if I peg Webpack at 1.15.x and Webpacker at ~> 1.0.
If a regular committer could help me out to get the tests passing on master then I could continue working here to get more Webpacker support.

We should really think about the maintainability of the Webpacker support as the manifest paths between versions seems really flakey and i'm not keen on the pattern of .responds_to? and defined? everywhere.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Master finally passes and this does mean Webpacker 3 support (And 1.1, 1.2, 2.0) on this branch.
I split out as much as I could do make it clearer but there is still a lot of mess I caused in the code, lots of refactoring to do.

@BookOfGregBookOfGreg mentioned this pull request Sep 12, 2017
6 tasks
@@ -1,4 +1,5 @@
require "open-uri"
# require 'pry'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rmosolgo

Copy link
Copy Markdown
Contributor

Wow, this is really impressive! Thanks for all your work in digging in on this. It's nice to see that Webpacker has provided some APIs to support this kind of thing, too.

I see the green check, and the tests look good. So, what's still to go on this one?

@ArnonHongklay

Copy link
Copy Markdown

👍

@BookOfGreg

BookOfGreg commented Sep 13, 2017

Copy link
Copy Markdown
ContributorAuthor

This could go in to master it's current state as I believe it to be an improvement but frankly this could use some TLC and refactoring.

I'm certain all my MAJOR < 3 checks could be extracted into a class following the existing pattern.
There are probably better ways of integrating with the Webpacker.dev_server also.

I found some odd behaviour such as webpack 3 seems to read the wrong node_modules folder when loading in ../../../../react_ujs, so I used the actual npm package for it instead, that might cause some people in the wild issues that I can't even predict if they're messing with node context.

All these tweaks could be in future patches to this though.

Edit: Since I've modified the webpacker 1.1, 1.2 and 2 support, it would be good to see if it still works without bugs on an existing project for confirmation.

@Melonbwead

Copy link
Copy Markdown

You beast @BookOfGreg 👍 ❗️

@BookOfGreg

BookOfGreg commented Sep 15, 2017

Copy link
Copy Markdown
ContributorAuthor

I think we should probably merge this if we can be confident that it works. All existing tests pass. I can carry on with a tidy up beyond this.

Edit: apparently linting broke the tests. How odd. Will keep looking into it.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Tests fail on specific seeds:
bundle exec appraisal rails-5_no_sprockets_webpacker_3 rake test TESTOPTS="--seed=17880"

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

@rmosolgo If you like, :shipit: and I'll keep PR'ing rubocops, test fixes and refactorings as patch-level changes after this. Be good to start getting some bug reports from folks so I know if this accidentally impacts anyone.

@rmosolgo

Copy link
Copy Markdown
Contributor

That sounds great, let me go ahead with a release!

@rmosolgo
rmosolgo merged commit 901f58d into reactjs:masterSep 18, 2017
@gaiapunk

gaiapunk commented Sep 20, 2017

Copy link
Copy Markdown

I'm still having the #778 issue and was wondering if this latest merge had been release to fix it or if there is a workaround on my end that I can do before a fix is released, thanks for all the work you do!

@BookOfGreg

BookOfGreg commented Sep 20, 2017

Copy link
Copy Markdown
ContributorAuthor

@gaiapunk It's not been released as a gem yet but it's possible to use the master branch directly with:
gem 'react-rails', git: 'https://github.com/reactjs/react-rails.git', branch: 'master'

@BookOfGreg
BookOfGreg deleted the fix-776 branch September 20, 2017 09:19
@BookOfGregBookOfGreg mentioned this pull request Sep 27, 2017
Sign up for freeto 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.

13 participants

@BookOfGreg@basicBrogrammer@tomwaddington@rmosolgo@ArnonHongklay@Melonbwead@gaiapunk@willcosgrove@swrobel@AnatoliiD@bradleesand@RiccardoMargiotta@vikash-k-singh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Support singleton webpacker (3 and above) - #777

Merged
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776
Sep 18, 2017
Merged

Support singleton webpacker (3 and above)#777
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776

Conversation

@BookOfGreg

@BookOfGregBookOfGreg commented Aug 31, 2017

Copy link
Copy Markdown
Contributor

Fixes#776
Fixes#775
Fixes#778
Fixes#780
Fixes#781
Fixes#782

Once again I could probably use some assistance with the appraisal gem but other than that should be OK.

Edit: This turned into a BIG refactor of the tests to split out support and I'll be honest my code isn't the tidiest. Still plenty of refactoring to go as I'm not sure I'm thrilled about how I've switched method definitions of Webpacker version numbers.

private

def webpack_configuration
Webpacker.respond_to?(:config) ? Webpacker.config : webpack_configuration

@AnatoliiDAnatoliiDSep 1, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should be

Webpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configuration

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Aaw whoops. Wondered why my test didn't pass XD Thanks

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've still got issues around Webpacker::Manifest.load, seems to be how I detect respond_to? may be wrong for that one.

@bradleesandbradleesand left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The install generator won't work because entry_path was removed from webpacker config.

Webpacker::Configuration.source_path
.join(Webpacker::Configuration.entry_path)
webpack_configuration.source_path
.join(webpack_configuration.entry_path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new singleton implementation of Webpacker::Configuration no longer has entry_path. Instead, Webpacker::Configuration.source_path.join(Webpacker::Configuration.entry_path) becomes Webpacker.config.source_entry_path

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have suggested these changes in issue #778 .Please have a look into it. I was trying to install react with rails and failed to run react installer command. I look into gem and found that method calling is not proper.

I fix this with minor changes in lib/generators/react/install_generator.rb file.

source_path is a instance method and was being called by class directly. Also entry_path was changed to source_entry_path in webpacker config. I have suggested these changes in #778

The main purpose for issue was #778 was to elevate exact issue and its solution along with it, as there was no fix until that time.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using webpacker 3.0.1 Webpacker.config.source_entry_path worked for me 👍

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks good, only a minor change suggested that webpack_configuration.entry_path should be changed to webpack_configuration.source_entry_path, as entry_path is renamed as source_entry_path in webpacker config file. File location for this file is lib/generators/react/install_generator.rb under method name javascript_dir.

@BookOfGreg

BookOfGreg commented Sep 5, 2017

Copy link
Copy Markdown
ContributorAuthor

I took on the code from #778 , thank you for that one! Makes it a little easier to update.

Currently working on trying to make the new config/webpacker.yml and it's env files play nicely with webpacker V1's paths.yml. It's putting my manifest in the webpacker.yml's output location with the paths.yml's manifest name so the webpacker helpers can't find the manifest.

Will take me some time to figure out all the subtleties of this before I'm done. Very welcome for someone to help with this!

Edit: Is there a nice way of changing the config files in test/dummy?

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In file lib/react/server_rendering/webpacker_manifest_container.rb, I am looking at a method name lookup_path at line no. 24, but I don't think it is defined anywhere in the both react-rails & webpacker gem. This will through an error once find_asset(logical_path) method is being called.

Please look into it! Are you guys loading any file with such variable?

Apart for this, hello @BookOfGreg, what kind of changes you wanna in test/dummy config files. Does it contains any Json data? Please share, hopefully I may change that!

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Webpacker 1 and 2 have lookup_path, Webpacker 3 does not. and I'll have to compose a method of finding the actual path of a file.

@basicBrogrammer

Copy link
Copy Markdown

I'm not sure if this is related, but the component generator is putting my components in app/javascript/packs/components. To get this to work, I had to change the require context from 'components' to './components'

app/javascript/packs/application.js
var componentRequireContext = require.context('./components', true')

And also in my es6 components I have to require('../components/NameOfComponent')
Any thoughts?

var NewList = function() { return <span>"New List"</span> }
HEREDOC
new_file_contents = <<-HEREDOC
var NewList = function() { return <span>"New List"</span> }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like you might've messed up the indentation here

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The <<- syntax allows the closing HEREDOC to be indented but it won't strip space from the content so this works just fine.

end

def config
!!defined?(Webpacker::Configuration) ? Webpacker::Configuration : Webpacker.config

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this needs to be

defconfigWebpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configurationend

Because even in Webpacker 3, the Webpacker::Configuration constant is defined.

@tomwaddington

Copy link
Copy Markdown

Just ran into issue #778, and this fixed it for me. 👍

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

I'll admit that I'm at a bit of a dead end. On #779 I've got all of Travis passing except Rails 5 + Webpacker, but on that branch Webpack builds to public/packs but Webpacker looks in packs/packs instead. That is the case even if I peg Webpack at 1.15.x and Webpacker at ~> 1.0.
If a regular committer could help me out to get the tests passing on master then I could continue working here to get more Webpacker support.

We should really think about the maintainability of the Webpacker support as the manifest paths between versions seems really flakey and i'm not keen on the pattern of .responds_to? and defined? everywhere.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Master finally passes and this does mean Webpacker 3 support (And 1.1, 1.2, 2.0) on this branch.
I split out as much as I could do make it clearer but there is still a lot of mess I caused in the code, lots of refactoring to do.

@BookOfGregBookOfGreg mentioned this pull request Sep 12, 2017
6 tasks
@@ -1,4 +1,5 @@
require "open-uri"
# require 'pry'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rmosolgo

Copy link
Copy Markdown
Contributor

Wow, this is really impressive! Thanks for all your work in digging in on this. It's nice to see that Webpacker has provided some APIs to support this kind of thing, too.

I see the green check, and the tests look good. So, what's still to go on this one?

@ArnonHongklay

Copy link
Copy Markdown

👍

@BookOfGreg

BookOfGreg commented Sep 13, 2017

Copy link
Copy Markdown
ContributorAuthor

This could go in to master it's current state as I believe it to be an improvement but frankly this could use some TLC and refactoring.

I'm certain all my MAJOR < 3 checks could be extracted into a class following the existing pattern.
There are probably better ways of integrating with the Webpacker.dev_server also.

I found some odd behaviour such as webpack 3 seems to read the wrong node_modules folder when loading in ../../../../react_ujs, so I used the actual npm package for it instead, that might cause some people in the wild issues that I can't even predict if they're messing with node context.

All these tweaks could be in future patches to this though.

Edit: Since I've modified the webpacker 1.1, 1.2 and 2 support, it would be good to see if it still works without bugs on an existing project for confirmation.

@Melonbwead

Copy link
Copy Markdown

You beast @BookOfGreg 👍 ❗️

@BookOfGreg

BookOfGreg commented Sep 15, 2017

Copy link
Copy Markdown
ContributorAuthor

I think we should probably merge this if we can be confident that it works. All existing tests pass. I can carry on with a tidy up beyond this.

Edit: apparently linting broke the tests. How odd. Will keep looking into it.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Tests fail on specific seeds:
bundle exec appraisal rails-5_no_sprockets_webpacker_3 rake test TESTOPTS="--seed=17880"

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

@rmosolgo If you like, :shipit: and I'll keep PR'ing rubocops, test fixes and refactorings as patch-level changes after this. Be good to start getting some bug reports from folks so I know if this accidentally impacts anyone.

@rmosolgo

Copy link
Copy Markdown
Contributor

That sounds great, let me go ahead with a release!

@rmosolgo
rmosolgo merged commit 901f58d into reactjs:masterSep 18, 2017
@gaiapunk

gaiapunk commented Sep 20, 2017

Copy link
Copy Markdown

I'm still having the #778 issue and was wondering if this latest merge had been release to fix it or if there is a workaround on my end that I can do before a fix is released, thanks for all the work you do!

@BookOfGreg

BookOfGreg commented Sep 20, 2017

Copy link
Copy Markdown
ContributorAuthor

@gaiapunk It's not been released as a gem yet but it's possible to use the master branch directly with:
gem 'react-rails', git: 'https://github.com/reactjs/react-rails.git', branch: 'master'

@BookOfGreg
BookOfGreg deleted the fix-776 branch September 20, 2017 09:19
@BookOfGregBookOfGreg mentioned this pull request Sep 27, 2017
Sign up for freeto 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.

13 participants

@BookOfGreg@basicBrogrammer@tomwaddington@rmosolgo@ArnonHongklay@Melonbwead@gaiapunk@willcosgrove@swrobel@AnatoliiD@bradleesand@RiccardoMargiotta@vikash-k-singh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Support singleton webpacker (3 and above) - #777

Merged
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776
Sep 18, 2017
Merged

Support singleton webpacker (3 and above)#777
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776

Conversation

@BookOfGreg

@BookOfGregBookOfGreg commented Aug 31, 2017

Copy link
Copy Markdown
Contributor

Fixes#776
Fixes#775
Fixes#778
Fixes#780
Fixes#781
Fixes#782

Once again I could probably use some assistance with the appraisal gem but other than that should be OK.

Edit: This turned into a BIG refactor of the tests to split out support and I'll be honest my code isn't the tidiest. Still plenty of refactoring to go as I'm not sure I'm thrilled about how I've switched method definitions of Webpacker version numbers.

private

def webpack_configuration
Webpacker.respond_to?(:config) ? Webpacker.config : webpack_configuration

@AnatoliiDAnatoliiDSep 1, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should be

Webpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configuration

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Aaw whoops. Wondered why my test didn't pass XD Thanks

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've still got issues around Webpacker::Manifest.load, seems to be how I detect respond_to? may be wrong for that one.

@bradleesandbradleesand left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The install generator won't work because entry_path was removed from webpacker config.

Webpacker::Configuration.source_path
.join(Webpacker::Configuration.entry_path)
webpack_configuration.source_path
.join(webpack_configuration.entry_path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new singleton implementation of Webpacker::Configuration no longer has entry_path. Instead, Webpacker::Configuration.source_path.join(Webpacker::Configuration.entry_path) becomes Webpacker.config.source_entry_path

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have suggested these changes in issue #778 .Please have a look into it. I was trying to install react with rails and failed to run react installer command. I look into gem and found that method calling is not proper.

I fix this with minor changes in lib/generators/react/install_generator.rb file.

source_path is a instance method and was being called by class directly. Also entry_path was changed to source_entry_path in webpacker config. I have suggested these changes in #778

The main purpose for issue was #778 was to elevate exact issue and its solution along with it, as there was no fix until that time.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using webpacker 3.0.1 Webpacker.config.source_entry_path worked for me 👍

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks good, only a minor change suggested that webpack_configuration.entry_path should be changed to webpack_configuration.source_entry_path, as entry_path is renamed as source_entry_path in webpacker config file. File location for this file is lib/generators/react/install_generator.rb under method name javascript_dir.

@BookOfGreg

BookOfGreg commented Sep 5, 2017

Copy link
Copy Markdown
ContributorAuthor

I took on the code from #778 , thank you for that one! Makes it a little easier to update.

Currently working on trying to make the new config/webpacker.yml and it's env files play nicely with webpacker V1's paths.yml. It's putting my manifest in the webpacker.yml's output location with the paths.yml's manifest name so the webpacker helpers can't find the manifest.

Will take me some time to figure out all the subtleties of this before I'm done. Very welcome for someone to help with this!

Edit: Is there a nice way of changing the config files in test/dummy?

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In file lib/react/server_rendering/webpacker_manifest_container.rb, I am looking at a method name lookup_path at line no. 24, but I don't think it is defined anywhere in the both react-rails & webpacker gem. This will through an error once find_asset(logical_path) method is being called.

Please look into it! Are you guys loading any file with such variable?

Apart for this, hello @BookOfGreg, what kind of changes you wanna in test/dummy config files. Does it contains any Json data? Please share, hopefully I may change that!

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Webpacker 1 and 2 have lookup_path, Webpacker 3 does not. and I'll have to compose a method of finding the actual path of a file.

@basicBrogrammer

Copy link
Copy Markdown

I'm not sure if this is related, but the component generator is putting my components in app/javascript/packs/components. To get this to work, I had to change the require context from 'components' to './components'

app/javascript/packs/application.js
var componentRequireContext = require.context('./components', true')

And also in my es6 components I have to require('../components/NameOfComponent')
Any thoughts?

var NewList = function() { return <span>"New List"</span> }
HEREDOC
new_file_contents = <<-HEREDOC
var NewList = function() { return <span>"New List"</span> }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like you might've messed up the indentation here

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The <<- syntax allows the closing HEREDOC to be indented but it won't strip space from the content so this works just fine.

end

def config
!!defined?(Webpacker::Configuration) ? Webpacker::Configuration : Webpacker.config

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this needs to be

defconfigWebpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configurationend

Because even in Webpacker 3, the Webpacker::Configuration constant is defined.

@tomwaddington

Copy link
Copy Markdown

Just ran into issue #778, and this fixed it for me. 👍

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

I'll admit that I'm at a bit of a dead end. On #779 I've got all of Travis passing except Rails 5 + Webpacker, but on that branch Webpack builds to public/packs but Webpacker looks in packs/packs instead. That is the case even if I peg Webpack at 1.15.x and Webpacker at ~> 1.0.
If a regular committer could help me out to get the tests passing on master then I could continue working here to get more Webpacker support.

We should really think about the maintainability of the Webpacker support as the manifest paths between versions seems really flakey and i'm not keen on the pattern of .responds_to? and defined? everywhere.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Master finally passes and this does mean Webpacker 3 support (And 1.1, 1.2, 2.0) on this branch.
I split out as much as I could do make it clearer but there is still a lot of mess I caused in the code, lots of refactoring to do.

@BookOfGregBookOfGreg mentioned this pull request Sep 12, 2017
6 tasks
@@ -1,4 +1,5 @@
require "open-uri"
# require 'pry'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rmosolgo

Copy link
Copy Markdown
Contributor

Wow, this is really impressive! Thanks for all your work in digging in on this. It's nice to see that Webpacker has provided some APIs to support this kind of thing, too.

I see the green check, and the tests look good. So, what's still to go on this one?

@ArnonHongklay

Copy link
Copy Markdown

👍

@BookOfGreg

BookOfGreg commented Sep 13, 2017

Copy link
Copy Markdown
ContributorAuthor

This could go in to master it's current state as I believe it to be an improvement but frankly this could use some TLC and refactoring.

I'm certain all my MAJOR < 3 checks could be extracted into a class following the existing pattern.
There are probably better ways of integrating with the Webpacker.dev_server also.

I found some odd behaviour such as webpack 3 seems to read the wrong node_modules folder when loading in ../../../../react_ujs, so I used the actual npm package for it instead, that might cause some people in the wild issues that I can't even predict if they're messing with node context.

All these tweaks could be in future patches to this though.

Edit: Since I've modified the webpacker 1.1, 1.2 and 2 support, it would be good to see if it still works without bugs on an existing project for confirmation.

@Melonbwead

Copy link
Copy Markdown

You beast @BookOfGreg 👍 ❗️

@BookOfGreg

BookOfGreg commented Sep 15, 2017

Copy link
Copy Markdown
ContributorAuthor

I think we should probably merge this if we can be confident that it works. All existing tests pass. I can carry on with a tidy up beyond this.

Edit: apparently linting broke the tests. How odd. Will keep looking into it.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Tests fail on specific seeds:
bundle exec appraisal rails-5_no_sprockets_webpacker_3 rake test TESTOPTS="--seed=17880"

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

@rmosolgo If you like, :shipit: and I'll keep PR'ing rubocops, test fixes and refactorings as patch-level changes after this. Be good to start getting some bug reports from folks so I know if this accidentally impacts anyone.

@rmosolgo

Copy link
Copy Markdown
Contributor

That sounds great, let me go ahead with a release!

@rmosolgo
rmosolgo merged commit 901f58d into reactjs:masterSep 18, 2017
@gaiapunk

gaiapunk commented Sep 20, 2017

Copy link
Copy Markdown

I'm still having the #778 issue and was wondering if this latest merge had been release to fix it or if there is a workaround on my end that I can do before a fix is released, thanks for all the work you do!

@BookOfGreg

BookOfGreg commented Sep 20, 2017

Copy link
Copy Markdown
ContributorAuthor

@gaiapunk It's not been released as a gem yet but it's possible to use the master branch directly with:
gem 'react-rails', git: 'https://github.com/reactjs/react-rails.git', branch: 'master'

@BookOfGreg
BookOfGreg deleted the fix-776 branch September 20, 2017 09:19
@BookOfGregBookOfGreg mentioned this pull request Sep 27, 2017
Sign up for freeto 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.

13 participants

@BookOfGreg@basicBrogrammer@tomwaddington@rmosolgo@ArnonHongklay@Melonbwead@gaiapunk@willcosgrove@swrobel@AnatoliiD@bradleesand@RiccardoMargiotta@vikash-k-singh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Support singleton webpacker (3 and above) - #777

Merged
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776
Sep 18, 2017
Merged

Support singleton webpacker (3 and above)#777
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776

Conversation

@BookOfGreg

@BookOfGregBookOfGreg commented Aug 31, 2017

Copy link
Copy Markdown
Contributor

Fixes#776
Fixes#775
Fixes#778
Fixes#780
Fixes#781
Fixes#782

Once again I could probably use some assistance with the appraisal gem but other than that should be OK.

Edit: This turned into a BIG refactor of the tests to split out support and I'll be honest my code isn't the tidiest. Still plenty of refactoring to go as I'm not sure I'm thrilled about how I've switched method definitions of Webpacker version numbers.

private

def webpack_configuration
Webpacker.respond_to?(:config) ? Webpacker.config : webpack_configuration

@AnatoliiDAnatoliiDSep 1, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should be

Webpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configuration

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Aaw whoops. Wondered why my test didn't pass XD Thanks

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've still got issues around Webpacker::Manifest.load, seems to be how I detect respond_to? may be wrong for that one.

@bradleesandbradleesand left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The install generator won't work because entry_path was removed from webpacker config.

Webpacker::Configuration.source_path
.join(Webpacker::Configuration.entry_path)
webpack_configuration.source_path
.join(webpack_configuration.entry_path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new singleton implementation of Webpacker::Configuration no longer has entry_path. Instead, Webpacker::Configuration.source_path.join(Webpacker::Configuration.entry_path) becomes Webpacker.config.source_entry_path

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have suggested these changes in issue #778 .Please have a look into it. I was trying to install react with rails and failed to run react installer command. I look into gem and found that method calling is not proper.

I fix this with minor changes in lib/generators/react/install_generator.rb file.

source_path is a instance method and was being called by class directly. Also entry_path was changed to source_entry_path in webpacker config. I have suggested these changes in #778

The main purpose for issue was #778 was to elevate exact issue and its solution along with it, as there was no fix until that time.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using webpacker 3.0.1 Webpacker.config.source_entry_path worked for me 👍

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks good, only a minor change suggested that webpack_configuration.entry_path should be changed to webpack_configuration.source_entry_path, as entry_path is renamed as source_entry_path in webpacker config file. File location for this file is lib/generators/react/install_generator.rb under method name javascript_dir.

@BookOfGreg

BookOfGreg commented Sep 5, 2017

Copy link
Copy Markdown
ContributorAuthor

I took on the code from #778 , thank you for that one! Makes it a little easier to update.

Currently working on trying to make the new config/webpacker.yml and it's env files play nicely with webpacker V1's paths.yml. It's putting my manifest in the webpacker.yml's output location with the paths.yml's manifest name so the webpacker helpers can't find the manifest.

Will take me some time to figure out all the subtleties of this before I'm done. Very welcome for someone to help with this!

Edit: Is there a nice way of changing the config files in test/dummy?

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In file lib/react/server_rendering/webpacker_manifest_container.rb, I am looking at a method name lookup_path at line no. 24, but I don't think it is defined anywhere in the both react-rails & webpacker gem. This will through an error once find_asset(logical_path) method is being called.

Please look into it! Are you guys loading any file with such variable?

Apart for this, hello @BookOfGreg, what kind of changes you wanna in test/dummy config files. Does it contains any Json data? Please share, hopefully I may change that!

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Webpacker 1 and 2 have lookup_path, Webpacker 3 does not. and I'll have to compose a method of finding the actual path of a file.

@basicBrogrammer

Copy link
Copy Markdown

I'm not sure if this is related, but the component generator is putting my components in app/javascript/packs/components. To get this to work, I had to change the require context from 'components' to './components'

app/javascript/packs/application.js
var componentRequireContext = require.context('./components', true')

And also in my es6 components I have to require('../components/NameOfComponent')
Any thoughts?

var NewList = function() { return <span>"New List"</span> }
HEREDOC
new_file_contents = <<-HEREDOC
var NewList = function() { return <span>"New List"</span> }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like you might've messed up the indentation here

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The <<- syntax allows the closing HEREDOC to be indented but it won't strip space from the content so this works just fine.

end

def config
!!defined?(Webpacker::Configuration) ? Webpacker::Configuration : Webpacker.config

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this needs to be

defconfigWebpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configurationend

Because even in Webpacker 3, the Webpacker::Configuration constant is defined.

@tomwaddington

Copy link
Copy Markdown

Just ran into issue #778, and this fixed it for me. 👍

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

I'll admit that I'm at a bit of a dead end. On #779 I've got all of Travis passing except Rails 5 + Webpacker, but on that branch Webpack builds to public/packs but Webpacker looks in packs/packs instead. That is the case even if I peg Webpack at 1.15.x and Webpacker at ~> 1.0.
If a regular committer could help me out to get the tests passing on master then I could continue working here to get more Webpacker support.

We should really think about the maintainability of the Webpacker support as the manifest paths between versions seems really flakey and i'm not keen on the pattern of .responds_to? and defined? everywhere.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Master finally passes and this does mean Webpacker 3 support (And 1.1, 1.2, 2.0) on this branch.
I split out as much as I could do make it clearer but there is still a lot of mess I caused in the code, lots of refactoring to do.

@BookOfGregBookOfGreg mentioned this pull request Sep 12, 2017
6 tasks
@@ -1,4 +1,5 @@
require "open-uri"
# require 'pry'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rmosolgo

Copy link
Copy Markdown
Contributor

Wow, this is really impressive! Thanks for all your work in digging in on this. It's nice to see that Webpacker has provided some APIs to support this kind of thing, too.

I see the green check, and the tests look good. So, what's still to go on this one?

@ArnonHongklay

Copy link
Copy Markdown

👍

@BookOfGreg

BookOfGreg commented Sep 13, 2017

Copy link
Copy Markdown
ContributorAuthor

This could go in to master it's current state as I believe it to be an improvement but frankly this could use some TLC and refactoring.

I'm certain all my MAJOR < 3 checks could be extracted into a class following the existing pattern.
There are probably better ways of integrating with the Webpacker.dev_server also.

I found some odd behaviour such as webpack 3 seems to read the wrong node_modules folder when loading in ../../../../react_ujs, so I used the actual npm package for it instead, that might cause some people in the wild issues that I can't even predict if they're messing with node context.

All these tweaks could be in future patches to this though.

Edit: Since I've modified the webpacker 1.1, 1.2 and 2 support, it would be good to see if it still works without bugs on an existing project for confirmation.

@Melonbwead

Copy link
Copy Markdown

You beast @BookOfGreg 👍 ❗️

@BookOfGreg

BookOfGreg commented Sep 15, 2017

Copy link
Copy Markdown
ContributorAuthor

I think we should probably merge this if we can be confident that it works. All existing tests pass. I can carry on with a tidy up beyond this.

Edit: apparently linting broke the tests. How odd. Will keep looking into it.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Tests fail on specific seeds:
bundle exec appraisal rails-5_no_sprockets_webpacker_3 rake test TESTOPTS="--seed=17880"

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

@rmosolgo If you like, :shipit: and I'll keep PR'ing rubocops, test fixes and refactorings as patch-level changes after this. Be good to start getting some bug reports from folks so I know if this accidentally impacts anyone.

@rmosolgo

Copy link
Copy Markdown
Contributor

That sounds great, let me go ahead with a release!

@rmosolgo
rmosolgo merged commit 901f58d into reactjs:masterSep 18, 2017
@gaiapunk

gaiapunk commented Sep 20, 2017

Copy link
Copy Markdown

I'm still having the #778 issue and was wondering if this latest merge had been release to fix it or if there is a workaround on my end that I can do before a fix is released, thanks for all the work you do!

@BookOfGreg

BookOfGreg commented Sep 20, 2017

Copy link
Copy Markdown
ContributorAuthor

@gaiapunk It's not been released as a gem yet but it's possible to use the master branch directly with:
gem 'react-rails', git: 'https://github.com/reactjs/react-rails.git', branch: 'master'

@BookOfGreg
BookOfGreg deleted the fix-776 branch September 20, 2017 09:19
@BookOfGregBookOfGreg mentioned this pull request Sep 27, 2017
Sign up for freeto 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.

13 participants

@BookOfGreg@basicBrogrammer@tomwaddington@rmosolgo@ArnonHongklay@Melonbwead@gaiapunk@willcosgrove@swrobel@AnatoliiD@bradleesand@RiccardoMargiotta@vikash-k-singh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Support singleton webpacker (3 and above) - #777

Merged
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776
Sep 18, 2017
Merged

Support singleton webpacker (3 and above)#777
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776

Conversation

@BookOfGreg

@BookOfGregBookOfGreg commented Aug 31, 2017

Copy link
Copy Markdown
Contributor

Fixes#776
Fixes#775
Fixes#778
Fixes#780
Fixes#781
Fixes#782

Once again I could probably use some assistance with the appraisal gem but other than that should be OK.

Edit: This turned into a BIG refactor of the tests to split out support and I'll be honest my code isn't the tidiest. Still plenty of refactoring to go as I'm not sure I'm thrilled about how I've switched method definitions of Webpacker version numbers.

private

def webpack_configuration
Webpacker.respond_to?(:config) ? Webpacker.config : webpack_configuration

@AnatoliiDAnatoliiDSep 1, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should be

Webpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configuration

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Aaw whoops. Wondered why my test didn't pass XD Thanks

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've still got issues around Webpacker::Manifest.load, seems to be how I detect respond_to? may be wrong for that one.

@bradleesandbradleesand left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The install generator won't work because entry_path was removed from webpacker config.

Webpacker::Configuration.source_path
.join(Webpacker::Configuration.entry_path)
webpack_configuration.source_path
.join(webpack_configuration.entry_path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new singleton implementation of Webpacker::Configuration no longer has entry_path. Instead, Webpacker::Configuration.source_path.join(Webpacker::Configuration.entry_path) becomes Webpacker.config.source_entry_path

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have suggested these changes in issue #778 .Please have a look into it. I was trying to install react with rails and failed to run react installer command. I look into gem and found that method calling is not proper.

I fix this with minor changes in lib/generators/react/install_generator.rb file.

source_path is a instance method and was being called by class directly. Also entry_path was changed to source_entry_path in webpacker config. I have suggested these changes in #778

The main purpose for issue was #778 was to elevate exact issue and its solution along with it, as there was no fix until that time.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using webpacker 3.0.1 Webpacker.config.source_entry_path worked for me 👍

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks good, only a minor change suggested that webpack_configuration.entry_path should be changed to webpack_configuration.source_entry_path, as entry_path is renamed as source_entry_path in webpacker config file. File location for this file is lib/generators/react/install_generator.rb under method name javascript_dir.

@BookOfGreg

BookOfGreg commented Sep 5, 2017

Copy link
Copy Markdown
ContributorAuthor

I took on the code from #778 , thank you for that one! Makes it a little easier to update.

Currently working on trying to make the new config/webpacker.yml and it's env files play nicely with webpacker V1's paths.yml. It's putting my manifest in the webpacker.yml's output location with the paths.yml's manifest name so the webpacker helpers can't find the manifest.

Will take me some time to figure out all the subtleties of this before I'm done. Very welcome for someone to help with this!

Edit: Is there a nice way of changing the config files in test/dummy?

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In file lib/react/server_rendering/webpacker_manifest_container.rb, I am looking at a method name lookup_path at line no. 24, but I don't think it is defined anywhere in the both react-rails & webpacker gem. This will through an error once find_asset(logical_path) method is being called.

Please look into it! Are you guys loading any file with such variable?

Apart for this, hello @BookOfGreg, what kind of changes you wanna in test/dummy config files. Does it contains any Json data? Please share, hopefully I may change that!

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Webpacker 1 and 2 have lookup_path, Webpacker 3 does not. and I'll have to compose a method of finding the actual path of a file.

@basicBrogrammer

Copy link
Copy Markdown

I'm not sure if this is related, but the component generator is putting my components in app/javascript/packs/components. To get this to work, I had to change the require context from 'components' to './components'

app/javascript/packs/application.js
var componentRequireContext = require.context('./components', true')

And also in my es6 components I have to require('../components/NameOfComponent')
Any thoughts?

var NewList = function() { return <span>"New List"</span> }
HEREDOC
new_file_contents = <<-HEREDOC
var NewList = function() { return <span>"New List"</span> }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like you might've messed up the indentation here

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The <<- syntax allows the closing HEREDOC to be indented but it won't strip space from the content so this works just fine.

end

def config
!!defined?(Webpacker::Configuration) ? Webpacker::Configuration : Webpacker.config

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this needs to be

defconfigWebpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configurationend

Because even in Webpacker 3, the Webpacker::Configuration constant is defined.

@tomwaddington

Copy link
Copy Markdown

Just ran into issue #778, and this fixed it for me. 👍

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

I'll admit that I'm at a bit of a dead end. On #779 I've got all of Travis passing except Rails 5 + Webpacker, but on that branch Webpack builds to public/packs but Webpacker looks in packs/packs instead. That is the case even if I peg Webpack at 1.15.x and Webpacker at ~> 1.0.
If a regular committer could help me out to get the tests passing on master then I could continue working here to get more Webpacker support.

We should really think about the maintainability of the Webpacker support as the manifest paths between versions seems really flakey and i'm not keen on the pattern of .responds_to? and defined? everywhere.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Master finally passes and this does mean Webpacker 3 support (And 1.1, 1.2, 2.0) on this branch.
I split out as much as I could do make it clearer but there is still a lot of mess I caused in the code, lots of refactoring to do.

@BookOfGregBookOfGreg mentioned this pull request Sep 12, 2017
6 tasks
@@ -1,4 +1,5 @@
require "open-uri"
# require 'pry'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rmosolgo

Copy link
Copy Markdown
Contributor

Wow, this is really impressive! Thanks for all your work in digging in on this. It's nice to see that Webpacker has provided some APIs to support this kind of thing, too.

I see the green check, and the tests look good. So, what's still to go on this one?

@ArnonHongklay

Copy link
Copy Markdown

👍

@BookOfGreg

BookOfGreg commented Sep 13, 2017

Copy link
Copy Markdown
ContributorAuthor

This could go in to master it's current state as I believe it to be an improvement but frankly this could use some TLC and refactoring.

I'm certain all my MAJOR < 3 checks could be extracted into a class following the existing pattern.
There are probably better ways of integrating with the Webpacker.dev_server also.

I found some odd behaviour such as webpack 3 seems to read the wrong node_modules folder when loading in ../../../../react_ujs, so I used the actual npm package for it instead, that might cause some people in the wild issues that I can't even predict if they're messing with node context.

All these tweaks could be in future patches to this though.

Edit: Since I've modified the webpacker 1.1, 1.2 and 2 support, it would be good to see if it still works without bugs on an existing project for confirmation.

@Melonbwead

Copy link
Copy Markdown

You beast @BookOfGreg 👍 ❗️

@BookOfGreg

BookOfGreg commented Sep 15, 2017

Copy link
Copy Markdown
ContributorAuthor

I think we should probably merge this if we can be confident that it works. All existing tests pass. I can carry on with a tidy up beyond this.

Edit: apparently linting broke the tests. How odd. Will keep looking into it.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Tests fail on specific seeds:
bundle exec appraisal rails-5_no_sprockets_webpacker_3 rake test TESTOPTS="--seed=17880"

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

@rmosolgo If you like, :shipit: and I'll keep PR'ing rubocops, test fixes and refactorings as patch-level changes after this. Be good to start getting some bug reports from folks so I know if this accidentally impacts anyone.

@rmosolgo

Copy link
Copy Markdown
Contributor

That sounds great, let me go ahead with a release!

@rmosolgo
rmosolgo merged commit 901f58d into reactjs:masterSep 18, 2017
@gaiapunk

gaiapunk commented Sep 20, 2017

Copy link
Copy Markdown

I'm still having the #778 issue and was wondering if this latest merge had been release to fix it or if there is a workaround on my end that I can do before a fix is released, thanks for all the work you do!

@BookOfGreg

BookOfGreg commented Sep 20, 2017

Copy link
Copy Markdown
ContributorAuthor

@gaiapunk It's not been released as a gem yet but it's possible to use the master branch directly with:
gem 'react-rails', git: 'https://github.com/reactjs/react-rails.git', branch: 'master'

@BookOfGreg
BookOfGreg deleted the fix-776 branch September 20, 2017 09:19
@BookOfGregBookOfGreg mentioned this pull request Sep 27, 2017
Sign up for freeto 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.

13 participants

@BookOfGreg@basicBrogrammer@tomwaddington@rmosolgo@ArnonHongklay@Melonbwead@gaiapunk@willcosgrove@swrobel@AnatoliiD@bradleesand@RiccardoMargiotta@vikash-k-singh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Support singleton webpacker (3 and above) - #777

Merged
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776
Sep 18, 2017
Merged

Support singleton webpacker (3 and above)#777
rmosolgo merged 50 commits into
reactjs:masterfrom
BookOfGreg:fix-776

Conversation

@BookOfGreg

@BookOfGregBookOfGreg commented Aug 31, 2017

Copy link
Copy Markdown
Contributor

Fixes#776
Fixes#775
Fixes#778
Fixes#780
Fixes#781
Fixes#782

Once again I could probably use some assistance with the appraisal gem but other than that should be OK.

Edit: This turned into a BIG refactor of the tests to split out support and I'll be honest my code isn't the tidiest. Still plenty of refactoring to go as I'm not sure I'm thrilled about how I've switched method definitions of Webpacker version numbers.

private

def webpack_configuration
Webpacker.respond_to?(:config) ? Webpacker.config : webpack_configuration

@AnatoliiDAnatoliiDSep 1, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should be

Webpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configuration

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Aaw whoops. Wondered why my test didn't pass XD Thanks

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've still got issues around Webpacker::Manifest.load, seems to be how I detect respond_to? may be wrong for that one.

@bradleesandbradleesand left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The install generator won't work because entry_path was removed from webpacker config.

Webpacker::Configuration.source_path
.join(Webpacker::Configuration.entry_path)
webpack_configuration.source_path
.join(webpack_configuration.entry_path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new singleton implementation of Webpacker::Configuration no longer has entry_path. Instead, Webpacker::Configuration.source_path.join(Webpacker::Configuration.entry_path) becomes Webpacker.config.source_entry_path

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have suggested these changes in issue #778 .Please have a look into it. I was trying to install react with rails and failed to run react installer command. I look into gem and found that method calling is not proper.

I fix this with minor changes in lib/generators/react/install_generator.rb file.

source_path is a instance method and was being called by class directly. Also entry_path was changed to source_entry_path in webpacker config. I have suggested these changes in #778

The main purpose for issue was #778 was to elevate exact issue and its solution along with it, as there was no fix until that time.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using webpacker 3.0.1 Webpacker.config.source_entry_path worked for me 👍

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks good, only a minor change suggested that webpack_configuration.entry_path should be changed to webpack_configuration.source_entry_path, as entry_path is renamed as source_entry_path in webpacker config file. File location for this file is lib/generators/react/install_generator.rb under method name javascript_dir.

@BookOfGreg

BookOfGreg commented Sep 5, 2017

Copy link
Copy Markdown
ContributorAuthor

I took on the code from #778 , thank you for that one! Makes it a little easier to update.

Currently working on trying to make the new config/webpacker.yml and it's env files play nicely with webpacker V1's paths.yml. It's putting my manifest in the webpacker.yml's output location with the paths.yml's manifest name so the webpacker helpers can't find the manifest.

Will take me some time to figure out all the subtleties of this before I'm done. Very welcome for someone to help with this!

Edit: Is there a nice way of changing the config files in test/dummy?

@vikash-k-singhvikash-k-singh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In file lib/react/server_rendering/webpacker_manifest_container.rb, I am looking at a method name lookup_path at line no. 24, but I don't think it is defined anywhere in the both react-rails & webpacker gem. This will through an error once find_asset(logical_path) method is being called.

Please look into it! Are you guys loading any file with such variable?

Apart for this, hello @BookOfGreg, what kind of changes you wanna in test/dummy config files. Does it contains any Json data? Please share, hopefully I may change that!

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Webpacker 1 and 2 have lookup_path, Webpacker 3 does not. and I'll have to compose a method of finding the actual path of a file.

@basicBrogrammer

Copy link
Copy Markdown

I'm not sure if this is related, but the component generator is putting my components in app/javascript/packs/components. To get this to work, I had to change the require context from 'components' to './components'

app/javascript/packs/application.js
var componentRequireContext = require.context('./components', true')

And also in my es6 components I have to require('../components/NameOfComponent')
Any thoughts?

var NewList = function() { return <span>"New List"</span> }
HEREDOC
new_file_contents = <<-HEREDOC
var NewList = function() { return <span>"New List"</span> }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like you might've messed up the indentation here

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The <<- syntax allows the closing HEREDOC to be indented but it won't strip space from the content so this works just fine.

end

def config
!!defined?(Webpacker::Configuration) ? Webpacker::Configuration : Webpacker.config

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this needs to be

defconfigWebpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configurationend

Because even in Webpacker 3, the Webpacker::Configuration constant is defined.

@tomwaddington

Copy link
Copy Markdown

Just ran into issue #778, and this fixed it for me. 👍

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

I'll admit that I'm at a bit of a dead end. On #779 I've got all of Travis passing except Rails 5 + Webpacker, but on that branch Webpack builds to public/packs but Webpacker looks in packs/packs instead. That is the case even if I peg Webpack at 1.15.x and Webpacker at ~> 1.0.
If a regular committer could help me out to get the tests passing on master then I could continue working here to get more Webpacker support.

We should really think about the maintainability of the Webpacker support as the manifest paths between versions seems really flakey and i'm not keen on the pattern of .responds_to? and defined? everywhere.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Master finally passes and this does mean Webpacker 3 support (And 1.1, 1.2, 2.0) on this branch.
I split out as much as I could do make it clearer but there is still a lot of mess I caused in the code, lots of refactoring to do.

@BookOfGregBookOfGreg mentioned this pull request Sep 12, 2017
6 tasks
@@ -1,4 +1,5 @@
require "open-uri"
# require 'pry'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rmosolgo

Copy link
Copy Markdown
Contributor

Wow, this is really impressive! Thanks for all your work in digging in on this. It's nice to see that Webpacker has provided some APIs to support this kind of thing, too.

I see the green check, and the tests look good. So, what's still to go on this one?

@ArnonHongklay

Copy link
Copy Markdown

👍

@BookOfGreg

BookOfGreg commented Sep 13, 2017

Copy link
Copy Markdown
ContributorAuthor

This could go in to master it's current state as I believe it to be an improvement but frankly this could use some TLC and refactoring.

I'm certain all my MAJOR < 3 checks could be extracted into a class following the existing pattern.
There are probably better ways of integrating with the Webpacker.dev_server also.

I found some odd behaviour such as webpack 3 seems to read the wrong node_modules folder when loading in ../../../../react_ujs, so I used the actual npm package for it instead, that might cause some people in the wild issues that I can't even predict if they're messing with node context.

All these tweaks could be in future patches to this though.

Edit: Since I've modified the webpacker 1.1, 1.2 and 2 support, it would be good to see if it still works without bugs on an existing project for confirmation.

@Melonbwead

Copy link
Copy Markdown

You beast @BookOfGreg 👍 ❗️

@BookOfGreg

BookOfGreg commented Sep 15, 2017

Copy link
Copy Markdown
ContributorAuthor

I think we should probably merge this if we can be confident that it works. All existing tests pass. I can carry on with a tidy up beyond this.

Edit: apparently linting broke the tests. How odd. Will keep looking into it.

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

Tests fail on specific seeds:
bundle exec appraisal rails-5_no_sprockets_webpacker_3 rake test TESTOPTS="--seed=17880"

@BookOfGreg

Copy link
Copy Markdown
ContributorAuthor

@rmosolgo If you like, :shipit: and I'll keep PR'ing rubocops, test fixes and refactorings as patch-level changes after this. Be good to start getting some bug reports from folks so I know if this accidentally impacts anyone.

@rmosolgo

Copy link
Copy Markdown
Contributor

That sounds great, let me go ahead with a release!

@rmosolgo
rmosolgo merged commit 901f58d into reactjs:masterSep 18, 2017
@gaiapunk

gaiapunk commented Sep 20, 2017

Copy link
Copy Markdown

I'm still having the #778 issue and was wondering if this latest merge had been release to fix it or if there is a workaround on my end that I can do before a fix is released, thanks for all the work you do!

@BookOfGreg

BookOfGreg commented Sep 20, 2017

Copy link
Copy Markdown
ContributorAuthor

@gaiapunk It's not been released as a gem yet but it's possible to use the master branch directly with:
gem 'react-rails', git: 'https://github.com/reactjs/react-rails.git', branch: 'master'

@BookOfGreg
BookOfGreg deleted the fix-776 branch September 20, 2017 09:19
@BookOfGregBookOfGreg mentioned this pull request Sep 27, 2017
Sign up for freeto 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.

13 participants

@BookOfGreg@basicBrogrammer@tomwaddington@rmosolgo@ArnonHongklay@Melonbwead@gaiapunk@willcosgrove@swrobel@AnatoliiD@bradleesand@RiccardoMargiotta@vikash-k-singh