FDC3 icon indicating copy to clipboard operation
FDC3 copied to clipboard

CJS build changes breaking Node consumers

Open enewsome opened this issue 6 months ago • 7 comments

Minor Issue

The recent build changes in v2.2.0 are causing runtime issues in Node. We have an internal library which is consumed by all applications, both in node and in the browser, and relies heavily on the finos package for leveraging its exported types and utilities. I see that the CommonJS output, that used to be built in, was broken out into a separate package. While we might be able to switch to that instead and update all references (both in this shared library and in apps that directly rely on finos), there are concerns around treeshaking of the cjs output for browser code.

Would it be possible to update the moduleResolution setting in the TS config to be either "node16" or "nodenext" and include the ".js" extension on imports so this can work in newer versions of Node? Alternatively if that isn't an option, can the build output be reverted to not break existing Node consumers of the package?

Area of Issue

  • [ ] App Directory
  • [ ] API
  • [ ] Context Data
  • [ ] Intents
  • [ ] Desktop Agent Bridging
  • [ ] Use Cases
  • [x] Other

enewsome avatar Jun 12 '25 15:06 enewsome

Hi @enewsome, apologies for the delay in responding to this - OSFF London 2025 has had many of the maintainers busy. This was discussed at #1618 (minutes for which are still pending). In short, yes we can make some changes to the CJS build. Ideally we'd confine changes to there rather than changing imports across the main FDC3 module, however, that may not be possible.

While we might be able to switch to that instead and update all references (both in this shared library and in apps that directly rely on finos), there are concerns around treeshaking of the cjs output for browser code.

Hopefully you just need to update the package.json to replace the dependency or add an override to point to the CJS module... Regarding the effect on tree shaking are you able to confirm and quantify that impact? If you are trying to run the same code in both node and browser, is it being built separately for each? If so could you simply use a override in the package.json for the node version of the code to switch to use the CJS module?

Would it be possible to update the moduleResolution setting in the TS config to be either "node16" or "nodenext" and include the ".js" extension on imports so this can work in newer versions of Node?

We don't currently set moduleResolution, module or target in the tsconfig, which we should probably be more explicit about (from a quick glance I'm not 100% sure what the defaults end up being). Hence, we'll need to figure out what the impact of any change would be on non-node apps, which is the primary use case for the module. I'm sure we'd be happy to receive and review a contribution on this - there are a lot of import statements to add the .js extensions to and we'll need to see the impact of the change on a typical tree-shaken app bundle.

kriswest avatar Jun 26 '25 07:06 kriswest

Hi Kris, circling back to this one. It may be possible for me to update those repositories as I originally mentioned but its a bit of a laborious task to find all the projects/repositories across FactSet that may be using that dependency in node and is on a version of build tooling that runs into this problem. I was hoping that since this was a intra major version breaking change that there was some way to patch this.

Regarding the effect on tree shaking are you able to confirm and quantify that impact? If you are trying to run the same code in both node and browser, is it being built separately for each?

Unfortunately I can't fully quantify the impact. My expertise in builds and build tooling has dwindled over the years but this was a concern from a colleague who was helping me investigate this issue originally and offering suggestions.

enewsome avatar Sep 19 '25 21:09 enewsome

Looking into this I think that there were a couple of issues with the setup when we moved to ES2022.

Changing the module to ES2022 changes the format of the modules that TS outputs. It will now output js that uses import statements rather than require statements. This does not mean that we are outputting ESM packages though. Without also changing the type field in the package.json files node still considers these files to be cjs files as the extension is just .js (not mjs). To fix the consumption of the modules in node I think we need to do 2 things:

  • mark the packages with "type": "module" to tell node that this is an ESM package
  • set the module resolution to "NodeNext" (this in turn requires that all our imports have a file extension)

I will raise a PR with these changes

Roaders avatar Sep 25 '25 16:09 Roaders

This issue is now affecting as as well. I am not 100% sure why we haven't seen this before but this stuff is hard to decipher and can result in odd behavior such as this.

Up until now we've had vitest running in our fdc3-web repo and that has been fine. Vitest is ESM so that ends up being ESM (our fdc3-web vitest instance) consuming a not properly configured library (@finos/fdc3). In this case Vitest seems to have figured it out.

Now we have our internal repo (lets call it internal-app) consuming our OSS fdc3-web repo that consumes the finos package:

internal-app (ESM) -> @morgan-stanley/fdc3-web (ESM) -> @finos/fdc3 (has issues)

I've found this before that directly consuming a package can be fine but consuming a package that has a transient dependency on another package that has issues can break things. That seems to be the situation that we are in now.

The error that I am seeing is:

Error: Cannot find module '/repo-path/node_modules/@finos/fdc3-context/dist/generated/context/ContextTypes' imported from /repo-path/node_modules/@finos/fdc3-context/dist/src/index.js

ContextTypes is definitely there but as it's not imported using a file extension vitest dies and we're left with most tests failing. If I add a .js extension:

export * from '../generated/context/ContextTypes.js';

to /repo-path/node_modules/@finos/fdc3-context/dist/src/index.js

then the error goes away (and is replaced with the next error of a similar nature.

I think that because of this we are going to change fdc3-web and our internal repo to use the commonjs version of fdc3 until we have a proper fix in the finos package.

Roaders avatar Oct 31 '25 15:10 Roaders

this might help anyone else out that is experiencing this problem. I have found a script that you can run that fixes the files in your node modules folder after installing fdc3:

npm install -D fix-esm-import-path

npx fix-esm-import-path --process-import-type  ./node_modules/@finos/fdc3-context && npx fix-esm-import-path --process-import-type  ./node_modules/@finos/fdc3-schema && npx fix-esm-import-path --process-import-type  ./node_modules/@finos/fdc3-standard && npx fix-esm-import-path --process-import-type  ./node_modules/@finos/fdc3-get-agent && npx fix-esm-import-path --process-import-type  ./node_modules/@finos/fdc3-agent-proxy

This works for me and at least means that I should be able to get builds running on our CI pipeline

Roaders avatar Nov 11 '25 08:11 Roaders

Keen to catch up on this topic @Roaders - I don't fully understand it as yet, but will ensure we discuss at the next relevant meeting. I'd like to bring it to ground and get fixes out in a 2.2.2 release.

kriswest avatar Nov 11 '25 11:11 kriswest

yes, I think we need to fix this and #1691 as soon as possible as the packages are currently broken and prevent people from using them in certain circumstances.

Roaders avatar Nov 11 '25 11:11 Roaders