feat(ui5-tooling-modules): respect cjs file extensions and try bundling them as well

[wridgeu]

Hey there 👋

this is my first PR in the ecosystem showcase so I hope things aren't too out of hand. Even though the actual change is quite small. 😅

I've had the issue (also described in a bit more detail further below) that one of my third party libraries (marked.js to be precise) swapped their build between versions 4.0.1 and 4.0.2 (this PR).

As far as I understand the require.resolve is able to successfully determine the dependency path, however in this particular case it is the .cjs build of marked. Looking through the code a bit more I realized that maybe using rollup within util.js could also solve my issues here. Sure enough by already using the rollup node resolve plugin, the proper esm build can be "pulled" or rather bundled up and be used accordingly. This fixes my issues during dev but also build time.

Here is some more information (including a few twitter threads that I created when looking into this ^^). Maybe this help to determine if this is even a valid approach or something else could've been done ...

• UI5 Module Loading - CJS instead of UMD/ESM - What could be wrong? wridgeu/github_pages#50

I haven't done a lot when it comes to this but I've been debugging the whole AST, Espree, Acorn stuff the past few days and was able to learn a bit at least. If there is anything I might have missed in my thoughts or if this is a bad idea or should be resolved differently then I'd love to know! Maybe then I should create an Issue rather than a PR. (:

I made sure that running yarn dev will result in a still functioning demo app and that it still successfully loads chart.js. ^^

Thanks for reading all this!

BR,
Marco

[mauriciolauffer]

.cjs files are explicit CommonJS modules, meaning they are built to run on Nodejs only, not browsers. Bundling .cjs could wreak havoc when parsing/executing the .js bundle file in the browser.

IMO, this PR should be rejected and closed.

Looking in the lib folder v4.0.2 you can see multiple "module" types and they should be used accordingly to the target environment and module management in place.

[wridgeu]

Hi @mauriciolauffer

thank you for the input! I thought this could lead to issues but wasn't sure (worked fine for me, in my case of course ^^), that's why I went with proposing this PR.

Looking in the lib folder v4.0.2 you can see multiple "module" types and they should be used accordingly to the target environment and module management in place.

I'm not sure in what way I could tell the UI5 tooling (in my case) to pull in the appropriate module type then. So far I simply relied on ui5-tooling-modules to do the job (previously to this, I used shimming a while back). But here .cjs is always being taken into account. Could this possibly have something to do with the way the .ts is being compiled, so basically something within the tsconfig? Maybe I'm looking for the fault in the wrong corner (because thats where the error messages lead me ^^), Any Input is greatly appreciated. (:

[mauriciolauffer]

I believe it has more to do with the way the module is resolved and consumed by ui5-tooling-modules.

See the entry points in marked's package.json. At this moment, ui5-tooling-modules seems to ignore it and uses the default nodejs entry main. That's something I've previously discussed with others from the community. However, no work done on this topic yet.

[wridgeu]

This makes so much sense(!) and is rather eye-opening and in line with what I read up and thought during the analysis of my problem. That also easily explains why the rollup node resolve part "just works". Thank you, much appreciated! (:

I was looking a bit into the require.resolve part at first and thought that in there a "solution" should be found as this seems to be the "root" of the issue. I'm not that experienced here so after some time I went on to then find out the, in this PR proposed, "solution" ... because it worked. ^^

As you already said, this PR could and probably should then be rejected & closed. I'm a bit curious however how this might go on / move forward. I'll keep looking if time allows and will obviously keep observing closely. :P

[petermuessig]

@mauriciolauffer @wridgeu regarding ignoring the main field of the package.json, this isn't true. The require.resolve call uses this information when just using the name of the dependency, e.g.:

package.json:

{
  "dependencies": {
    "chart.js": "*"
  }
}

SomeJSFile.js:

import { Chart } from "chart.js"

``:

  "main": "dist/chart.js",
  "module": "dist/chart.esm.js",

This is the benefit of the usage of the require.resolve API that I don't need to take care of which entry point to use.

[petermuessig]

One thing which we could try to leverage the browser field of the package.json

[mauriciolauffer]

@petermuessig indeed, following the browser, if available, as default is better.

PS: I didn't say it's ignoring main, I said it uses the default nodejs entry main 😋

[wridgeu]

One thing which we could try to leverage the browser field of the package.json

This sounds great. Although I wouldn't know how to go about this for now. From what I have read though this is not possible with the "native" require.resolve from Node. So I guess it is custom development or a third party lib (i.e. one of the bundlers modules or similar that implements an alternative node resolution algorithm)?

Maybe I've read the docs wrong but I'll keep checking back on this & reading up on it if time allows it. Thanks for the feedback to both of you, much appreciated & a lot learned. :)

I'd say we could close this PR then and open up an Issue instead. ^^

[wridgeu]

I've made an Issue out of this, thanks for the feedback! Learned a lot. :)

Issue: #602