Skip to content

filter by descriptionData #456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
KutnerUri opened this issue Aug 1, 2021 · 15 comments
Closed

filter by descriptionData #456

KutnerUri opened this issue Aug 1, 2021 · 15 comments
Labels
enhancement New feature or request

Comments

@KutnerUri
Copy link

I need more granular filtering, using the descriptionData rule available in Webpack5. (We have some packages in the node_modules that need reloading)
I see only exclude and include. Is support for the entire Webpack condition object coming?

And is it important to filter files for the ReactRefreshWebpackPlugin? or rule for react-refresh/babel enough?

@pmmmwh
Copy link
Owner

pmmmwh commented Aug 8, 2021

In theory it is possible to support this, but I don't rank this high on the list of priorities for 0.5.0 so it likely wouldn't come in the next stable version (as it needs some rework on options parsing - that part in Webpack 5 is completely re-written).

And is it important to filter files for the ReactRefreshWebpackPlugin? or rule for react-refresh/babel enough?

The include and exclude options are used for this plugin to determine whether the files need to be ran through the loader of this plugin, if you really need to HMR node_modules then yes you would have to make this plugin inject runtime for those files as well.

In the meantime you could workaround this by using capture groups in RegExp to specify a few modules I believe: for example, set exclude to /node_modules\/(?!some-dependency).*/.

@KutnerUri
Copy link
Author

KutnerUri commented Aug 9, 2021

ok. Looking forward for support, when you get to it.

I meant - what if I don't filter at all with the plugin? does it apply additional processing to the files, or increase bundling time? Since I'm already using react-refresh/babel, it seems like I could do something like this:

module: {
  rules: [
    {
      test: /\.js$/,
      include: path.join(workDir, 'node_modules'),
      descriptionData: { componentId: ComponentID.isValidObject },
      // and other filtering logic
      use: [
        {
        loader: require.resolve('babel-loader'),
        options: {
          plugins: [ require.resolve('react-refresh/babel') ],
          babelrc: false, configFile: false, compact: false, minified: false
        },
        },
      ],
    }
  ]
},
plugins: [
  new ReactRefreshWebpackPlugin({
    include: componentsDirs,
    exclude: /exclude_nothing_at_all/i,
  }),
]

For the workaround - I used exclude: /react-refresh-webpack-plugin/i, (or otherwise it would ignore node_modules by default).

@KutnerUri
Copy link
Author

I'm seeing more usecases where it is needed. In the example above, we used include: componentsDirs to apply the plugin only to our packages in the node_modules.
This overrides the default value (/\.([cm]js|[jt]sx?|flow)$/i), and applies to irrelevant files as well (like avatar.svg).

I can't add the file suffix rule into the include, so I have to do something like this: 😥

include: componentsDirs,
exclude: [/react-refresh-webpack-plugin/i, /(?<!\.js)$/],

I will try to find something else

@pmmmwh pmmmwh added the enhancement New feature or request label Sep 11, 2021
@pmmmwh
Copy link
Owner

pmmmwh commented Sep 28, 2021

I'm starting to work on improving this, expect it to be in the next patch release 😄

@KutnerUri
Copy link
Author

amazing! and it will support the same structure as a Webpack 5 rule?

@pmmmwh
Copy link
Owner

pmmmwh commented Sep 29, 2021

amazing! and it will support the same structure as a Webpack 5 rule?

Yes, hopefully it is possible

@KutnerUri
Copy link
Author

👀

@pmmmwh
Copy link
Owner

pmmmwh commented Nov 7, 2021

I've been trying to make this work, but unfortunately it is not as simple as it sounds.

The compiler for rules which Webpack use is not exposed for third parties, so to emulate their behaviour would mean we have to re-implement the whole thing.

Instead of going down this route, it is possible for you to opt out of the built-in integration of the loader by making the include part match nothing (in the next release there will be a kill switch option) and inject @pmmmwh/react-refresh-webpack-plugin/loader on your own via the module.rules property in your Webpack configuration. The loader is not really documented as it is considered part of our internal API, but if you don't use complex ESM builds the default options should work for you.

@KutnerUri
Copy link
Author

cool. I will try it out! And I still need to include the babel loader, right?

Actually it makes more sense, in an inversion of control kind of way - it is clearer what gets included, and has less "black magic". I'll dig a little in the source code, but do document it!

@pmmmwh
Copy link
Owner

pmmmwh commented Nov 8, 2021

Yes, you still need the Babel loader.

@KutnerUri
Copy link
Author

it works like a charm!
I actually like this approach, it might even solve some bugs I got from incorrect order of loader.
Maybe make this the default? :)

The options I see are const, esModule. Documentation would be appreciated. (especially since we want to move to ESM soon-ish)

@KutnerUri
Copy link
Author

I'll report if I run into problems..

@pmmmwh
Copy link
Owner

pmmmwh commented Nov 17, 2021

Maybe make this the default? :)

I still believe that the batteries-included and minimal configuration would be best, so the default of auto-injection seems to still make sense. However, it would be nice to add a section in the troubleshooting docs where this setup is documented.

The options I see are const, esModule. Documentation would be appreciated. (especially since we want to move to ESM soon-ish)

Yea, they will be there soon.

GiladShoham pushed a commit to teambit/bit that referenced this issue Nov 22, 2021
- use fast refresh loader explicitly, with the existing Webpack heuristics! Thanks to pmmmwh/react-refresh-webpack-plugin#456
- remove hacks trying to set which packages / components should be included by the plugin.
@raphael10-collab
Copy link

raphael10-collab commented Sep 22, 2022

@pmmmwh

I'm getting the same error message:

src/renderer/views/app/components/App/hypothesis_client/sidebar/components/HypothesisApp.js
File was processed with these loaders:
 * ./node_modules/@pmmmwh/react-refresh-webpack-plugin/loader/index.js
You may need an additional loader to handle the result of these loaders.
| 
|   return (
>   

This is the code:

  return (
    <div
      className={classnames(
        'h-full min-h-full overflow-scroll',
        // Precise padding to align with annotation cards in content
        // Larger padding on bottom for wide screens
        'lg:pb-16 bg-grey-2',
        'js-thread-list-scroll-root',
        {
          'theme-clean': isThemeClean,
          // Make room at top for the TopBar (40px) plus custom padding (9px)
          // but not in the Notebook, which doesn't use the TopBar
          'pt-[49px]': route !== 'notebook',
          'p-4 lg:p-12': route === 'notebook',
        }
      )}
      data-testid="hypothesis-app"
      style={backgroundStyle}
    >

Other Info:

"@pmmmwh/react-refresh-webpack-plugin": "^0.5.7"
 "@types/react": "17.0.11",
 "@types/react-dom": "17.0.8",
 node: v16.15.1
No react-scripts
O.S. : Ubuntu 20.04 Desktop

@pmmmwh
Copy link
Owner

pmmmwh commented Sep 26, 2022

@pmmmwh

I'm getting the same error message:

You have to make sure your JSX is transformed by your Webpack config. This error shows that your loader chain only includes this plugin but not anything else that would transform JSX into JS which is what Webpack needs to work with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants