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: force esm to reload the new module instead of the cached one #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AllanOricil
Copy link

@AllanOricil AllanOricil commented Sep 1, 2024

Without this change, config files stored as esm (the ones that use the dynamicImport loader) won't be updated. Esm returns the previous loaded state of the module.

How to visualize the bug

  1. create a config file using .mjs.
  2. Write a simple index.js which attaches a file watcher to your config file.
  3. Inside the file watch handler, load or search the config again. Do not recreate the lilconfig object. Use a singleton approach.
  4. Do some changes in your config file, then save.
  5. Verify that the watcher handler reloaded the config file, but the the state has not changed. Add a debugger inside dynamicImport to better visualize it.
  6. Apply these PR changes
  7. Rerun the above steps and now verify that the new state of the module was reloaded.

@AllanOricil
Copy link
Author

@antonk52 could you take a look, please.

@antonk52
Copy link
Owner

antonk52 commented Sep 3, 2024

Hey, thanks for the PR. I am hesitant to merge this in the current state. Since I last checked it is a known limitation of ESM in node that you cannot clear the module cache nodejs issue. Not sure if something changed in this regard. Currently we pass the module id as is to the loader which can result in an error if the id already contains any query params. So my main questions are:

  1. Is there a recommended nodejs approach to clear module cache instead of an arbitrary timestamp query param?
  2. If there is no other way, we need to ensure that it does not conflict with existing query params if any
  3. Adding this change will introduce a discrepancy to how modules load in sync vs async loaders. A similar functionality is needed for loaders that use require

@AllanOricil
Copy link
Author

AllanOricil commented Sep 3, 2024

@antonk52 I totally forgot that bundlers preprocessors could also add query params to the id to force invalidation for browser builds. Good catch.

  1. I only found solutions for browsers. In a static site build, for example, when we configure our bundlers to keep original file names, we have to append some sort of build id or timestamp to the file name at build time, to force the browser to invalidate cached modules. When file names don't change between builds, we use query params in <script> tags. So, I thought Node would do the same. I did it, and it worked for my use case, but I totally forgot that I can't just append something because there could exist other query params in the ID already. However, I did not researched a recommended way for Node. I believe it will follow the same behavior as a browser runtime. But I will google and post findings here.

  2. I will research if an url parser can do the job. Maybe there is a module Id parser too.

  3. this change will only focus on the "dynamicImport" loader, for async loads, and for esm modules. Can we think about require in a another pr?

@AllanOricil
Copy link
Author

Apparently require is being changed to work with esm

nodejs/help#2751

require exposes its cache and we can delete the current entry before reloading the newest. Maybe when it is ready, it can be used as the default for the dynamic loader. But again, nobody talked about a "solution" for import

@antonk52
Copy link
Owner

antonk52 commented Sep 4, 2024

  1. I will research if an url parser can do the job. Maybe there is a module Id parser too.

There is a built in Url and SearchParams that can utilised for this reason.

  1. this change will only focus on the "dynamicImport" loader, for async loads, and for esm modules. Can we think about require in a another pr?

I don't want to add discrepancy between async and sync loaders. For this reason adding a cache invalidation PR should target both loaders.


As there is currently no recommended way to do this in nodejs can this be a user provided loader instead? We can add this recommendation to the package documentation

@AllanOricil
Copy link
Author

AllanOricil commented Sep 5, 2024

@antonk52 I agree with that being a custom loader. Where would be the best place to put an example? Right after the yamls loader or in the section where you talk about ESM?

If I provide a custom loader for dynamic imports, will the "id" of the module be available in the loader's scope? Or do we need to change something to make it available?

@antonk52
Copy link
Owner

antonk52 commented Sep 5, 2024

Where would be the best place to put an example? Right after the yamls loader or in the section where you talk about ESM?

You can add a section about reloading modules and provide an example how to defined a loader that appends ?tm=Date.new to a filepath. Something along these lines.

import {lilconfig} from 'lilconfig'

function jsLoader(filepath)
  return import(`${filepath}?_lilconfig_ts=${Date.now()}`)
}

const options = {
    loaders: {
        '.js': jsLoader,
        '.mjs': jsLoader,
        '.cjs': jsLoader,
    }
}

lilconfig('myapp', options).search()
lilconfig('myapp', options).load(filepath)

If I provide a custom loader for dynamic imports, will the "id" of the module be available in the loader's scope? Or do we need to change something to make it available?

Overriding loaders is already available as an api. In your project you can provide a custom function for any extension and lilconfig will use it to load the content, this function takes filepath and filecontent and returns the module.

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.

2 participants