Skip to content
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

fix: Clear correct cache when compiling with Webpack #838

Merged
merged 2 commits into from
Sep 26, 2021

Conversation

Mordred
Copy link
Contributor

@Mordred Mordred commented Sep 23, 2021

Summary

Using module.hot is forcing me to use Webpack HotModuleReplacementPlugin and excluding @loadable/server from nodeExternal() so that module.hot is present. (8f10896)

Then __non_webpack_require__ is used in smartRequire (https://github.com/gregberge/loadable-components/blob/main/packages/server/src/util.js#L30) but clearModuleCache is clearing cache from require not from __non_webpack_require__.

This PR should fix this problem.

Test plan

See #821

@theKashey
Copy link
Collaborator

Sorry, took some time to connect all the dots and pieces and understand how it's working, or how it's not working :)

You are absolutely correct, smartRequire and clearCache should use the same "source" for module information.

@@ -1,5 +1,6 @@
export const clearModuleCache = moduleName => {
const m = require.cache[moduleName]
const cache = (typeof __non_webpack_require__ !== 'undefined' ? __non_webpack_require__ : require).cache;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for being picky, but let's safe future developers time and make it right this time

Let's use "single source" for both operations if the HAVE to be synchronized.

const getRequire = () => typeof __non_webpack_require__ !== 'undefined' ? __non_webpack_require__ : require;

export const clearModuleCache = moduleName => {
  const cache = getRequire().cache; // 👈
...
}

export const smartRequire = modulePath => {
  if (process.env.NODE_ENV !== 'production' && module.hot) {
    clearModuleCache(modulePath)
  }
 
  return getRequire(moduleName);  // 👈
}

Copy link
Collaborator

@theKashey theKashey left a comment

Choose a reason for hiding this comment

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

The approach is correct. Let's make it ideal

@Mordred
Copy link
Contributor Author

Mordred commented Sep 24, 2021

@theKashey OK, no problem. I've refactored it based on your suggestions and kept eval - not sure if it is necessary, but it was there in original code.

@theKashey theKashey merged commit 3b9cb20 into gregberge:main Sep 26, 2021
@theKashey
Copy link
Collaborator

thank you for your contribution, I will release the update shortly.

@kiyasov
Copy link

kiyasov commented Oct 5, 2021

thank you for your contribution, I will release the update shortly.

Hi! When will it be released?

@Dattaya Dattaya mentioned this pull request Oct 8, 2021
@rtymchyk
Copy link

rtymchyk commented Nov 1, 2021

Would appreciate a release with this commit when possible 🙏

@rtymchyk
Copy link

rtymchyk commented Dec 9, 2021

🦗

@theKashey
Copy link
Collaborator

🙄😅

@theKashey
Copy link
Collaborator

https://github.com/gregberge/loadable-components/releases/tag/v5.15.2

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

Successfully merging this pull request may close these issues.

None yet

4 participants