-
Notifications
You must be signed in to change notification settings - Fork 132
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
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? |
Sorry to clarify my previous response: I mentioned the idea to publish two modules, ES6 and CommonJS, allowing for a simple resolution for anyone's build that gets broken by switching to ES6 only and there were no objections. I think @novavi liked the idea of being able to track usage and retire the CommonJs module when its not in use. Hence, I think we should proceed as we've discussed offline @robmoffat, and it looks like you are on your way there already. |
yeah think this is done as discussed. WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a changelog adding (for the siwtch to searpate ES6 and commonJS modules).
Also, these packages will (I assume!) publish with the readme file from each package. fdc3-commonjs
is lacking a Readme file currently as is fdc3
, see https://www.npmjs.com/package/@kite9/fdc3 for what that looks like.
In the past the NPM package has inherited the repositoy's README file, which is not ideal. This is good oppurtunity to develop something more relevant, including content for both App and Desktop Agent developers as very basic getting started content. Each readme should also include an early reference to the other package and an explanation of which to use.
Co-authored-by: Kris West <kris.west@interop.io>
1 similar comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Kris West <kris.west@interop.io>
Co-authored-by: Kris West <kris.west@interop.io>
Co-authored-by: Kris West <kris.west@interop.io>
Co-authored-by: Kris West <kris.west@interop.io>
This is to complete #1297 and address #1351, adds rollup to the
fdc3
package 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/demo
process 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/old
dir) but I think a lot of the assumptions of that are not necessarily relevant anymore, perhaps?