Fdc3 for web impl commonjs #1452
Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
|
|
|
|
The old config built two sets of files into the module, which consumers could switch between by setting a NODE_ENV variable, which this config won't produce. I'm no export on these config, but it also looks like you are only producing common.js files, and in their development form (all comments etc. retained as terser is not used to dtrip them out). |
|
|
|
HI @kriswest, Yes that's right. The main difference between the two versions being that the production one was minified using terser. Based on @thorsent's feedback about this, it seems like the use case for CJS is automated tests on very legacy versions of node. Given that, it seemed best that we provide the un-minified version by default so at least people can figure out where their tests are going wrong. |
|
One of the participating teams fed back that they were still using CommonJS in builds. The advantage of having both types in the module is that, based on whether you are doing a dev or prod build, it includes the relevant version. If you then bundle and treeshake your own app you app code only then includes the version you use, so prod builds are leaner, while dev builds are unminimized, including comments, maps etc.. I have no idea if an app can do that minification itself if we don't. At least thats my (limited) understanding. @julianna-ciq probably understands it better? |
|
Do we actually know who is using CJS? It might be simpler to just ask that one person! I have a feeling this is super-niche and not worth spending a lot of time on, but at the same time it'd be nice not to give that one person a headache. |
|
@Roaders commented in a meeting - however, we don't know every app and team thats consuming the FDC3 npm module. Its gets around 8k downloads a week from NPM... |
|
The reason it's so different is that basically I had to start over and read the rollup docs on how to do this - the original script (preserved) didn't work out-of-the-box. As I looked into it, it seemed like a lot of the original rollup config probably wasn't relevant to what we were doing anymore. For example, it referenced react, but we don't have any react in the fdc3 module. So it's tricky. We could spend effort trying to preserve the NODE_ENV stuff if that's what you want? But (going by @thorsent's notes) it seems like an odd thing to have in there just to reduce the size of a download for the node environment, especially given the node environment is going to be pulling in the entire dependency into it's |
|
The react reference is only a minor part of that config (a couple of lines), the majority is related to preparing the variations on the module. I'm not convinced its producing both CommonJs and ES6 versions now. Also, the goal is not to reduce the module size downloaded by NPM, but rather the output bundle size of a project consuming the module. The module is going to increase in size with 2.2 and the loss of a minified version to pull in may exacerbate that increase in size for downstream projects. I think we need to understand the impact of changes to the module, before releasing a version with those changes. |
|
The react part of the config is probably inherited from tsdx's rollup config, as that is what was originally used to build the module (and also where the dev/prod build setup comes from). |
|
@kriswest did this get raised in the SWG meeting last week at all? |
|
I mentioned it, few opinions were offered and no conclusion drawn unfortunately |
|
Shall I close this then? |
This is to complete #1297 and address #1351, adds rollup to the
fdc3package so that common-js is still supported.Since I know CJS is used in node, I ran the following:
From within the
fdc3-for-web/demoprocess and got back:Which looks ok to me? Obviously, I can't call getAgent, so not sure how further to test this.
Nb, this is very much simplified from the original
rollup.config.js(preserved in thepackages/fdc3/olddir) but I think a lot of the assumptions of that are not necessarily relevant anymore, perhaps?