Skip to content
This repository was archived by the owner on Feb 26, 2024. It is now read-only.

Conversation

@acuarica
Copy link

@acuarica acuarica commented Nov 22, 2022

PR description

This PR solves an issue related with TruffleConfig.load when @truffle/config is used as a package. The issue happens when re-requiring Truffle config files in webpacked versions. More specifically, when trying to require the same config file more than once, the second time does not reflect any changes made since first the require.

The solution involves using the same original-require to both invalidate the require cache and re-load the Truffle config file. The problem was that original-require was only used to load the Truffle config file, but not to invalidate its cache.

Background

The delete cache was in from the very beginning, see this blame https://github.com/trufflesuite/truffle/blame/15e7eb87d4659f7b520ea1658e9a833f132567d4/packages/truffle-config/index.js#L244.

And in this commit fc455d9, the require("require-nocache") was removed and replaced with the current code.

Testing instructions

The issue with testing this PR end-to-end is that it only manifest in the bundled version, but not using the CLI.. For this reason I've created a gist containing a sample repo

https://gist.github.com/acuarica/c9c1ae1e4da22ef78d56b48e9757a67c

The unbundled and bundle versions return different results. The unbundled version works as expected, i.e., reflects changes made to Truffle config, however the bundled version does not. You can check this by running the following in the above repo

yarn bundle

and then

node main.js
node main-bundle.js

should have the same output, but displays different networks.

For completion, a unit test is included to check for re-loading config files.

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if documentation updates are required.

Breaking changes and new features

  • I have added any needed breaking-change and new-feature labels for the appropriate packages.

@cds-amal
Copy link
Contributor

I wonder if Truffle really needs to invalidate the cache. PR #5734 passing CI indicates the code path isn't covered in our tests. vscode-ext is likely the first legitimate user of this feature.

@acuarica
Copy link
Author

I wonder if Truffle really needs to invalidate the cache.

Good point. Not sure about the requirements for Truffle, but the fact that they have delete require.cache[...] makes me think that they want to account for this case.

@cds-amal
Copy link
Contributor

cds-amal commented Dec 2, 2022

I wonder if Truffle really needs to invalidate the cache.

Good point. Not sure about the requirements for Truffle, but the fact that they have delete require.cache[...] makes me think that they want to account for this case.

There's an opportunity to remove cache invalidation because this never worked in production, and no one complained about it until vscode-ext. Removing the cache invalidation constraint might be beneficial to Truffle, and I'd like to investigate that possibility (#5734).

@eggplantzzz
Copy link
Contributor

So for now I'm going to close this PR as we decided to remove the cache invalidation altogether as it is probably unnecessary.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants