Skip to content
This repository has been archived by the owner on Feb 1, 2022. It is now read-only.

Fix: do not cache require, require is already cached in NodeJS by design #194

Merged
merged 2 commits into from
Aug 7, 2020

Conversation

Codeneos
Copy link
Contributor

@Codeneos Codeneos commented Jul 7, 2020

In deps.ts several dependencies are being cached to a local object. As NodeJS by design caches require calls the caching done in deps.ts does not offer performance benefit. Instead it causes problems when using cli-ux together with webpack or rollup for which at this time there is no simple fix.

I think it might be better to replace all calls to deps.ts with standard require calls, but as that is a bigger change I first wanted to check if that is appriciated before I will start making those changes :)

…an additional caching layer (deps.ts) on top of that has no value and only breaks JS packaging tools such as webpack/rollup.
@Codeneos
Copy link
Contributor Author

Codeneos commented Jul 7, 2020

I see the build failed due to node/no-missing-require rules triggering on the require calls in deps.ts - these look like false positives that were previously hidden as require was wrapped in fetch. Looking at the errors they seem to be caused by the modules being required being typescript (.ts) files and not javascript (.js)

@RasPhilCo
Copy link
Contributor

Add eslint ignore statements should and I can merge it!

…lint not detecting ts files as valid js modules.
@Codeneos
Copy link
Contributor Author

Codeneos commented Aug 7, 2020

Done! I added the ignore statement for the whole file as most of the includes are affected by this.

@RasPhilCo RasPhilCo merged commit 99b9466 into oclif:master Aug 7, 2020
oclif-bot added a commit that referenced this pull request Aug 7, 2020
## [5.4.10](v5.4.9...v5.4.10) (2020-08-07)

### Bug Fixes

* do not cache require, require is already cached in NodeJS by design ([#194](#194)) ([99b9466](99b9466))
@oclif-bot
Copy link
Contributor

🎉 This PR is included in version 5.4.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants