Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Currently this PR successfully builds all packages but it does not run the demo app when The I still need to get the demo server working correctly but I ran out of time today. |
|
I imagine this PR will be a merge nightmare as it has touched so many files.... I don't know what the best way to deal with that will be. |
I don't think it will be that bad as most of the changes are in the imports into files. It would pick up conflicts with new additions, but won't with many other PRs. Regardless, we should try and get this done and merged ASAP. @hughtroeger has offered to try and get this tested as a solution for @enewsome's issue #1609 However, the tests / cucumber see quite unhappy with the change and need further work. The SAST scan failing is down to the rename of a script file that needs to be in .semgrepignore here: Lines 4 to 5 in 16100f5 |
|
yes I think that the tests will be an issue. Converting to ESM is usually fairly painless other than things liks jest / karma / webpack / ts-node. I have already replaced ts-node with tsx as it's much happier with esm but have no idea how hard it will be to convince the testing framework to work - especially as I have no experience with this test runner. |
|
I've spent a bit more time on this today and it's not good news I am afraid.... This could end up being a much bigger job that we first thought. |
|
I have confirmed that the fixes that I have put into this PR so far have fixed my issues when trying to run vitest in my local app. This is the case where my lib depends on The fix is good.... we "just" need to get the rest of the tooling in the mono repo working in an esm environment. |
Many thanks for your work on this issue @Roaders! @hughtroeger and @enewsome it would be worth confirming if this branch resolves some of your issues... @Roaders which tooling most needs looking at?
|
|
Also want to express thanks for working on this @Roaders. We will test the changes in this PR and get back to you shortly. |
|
I'm afraid that to test it you're going to have to check it out locally and build it locally then install each of the built packages one by one in your app. This worked for me. It's the tests that are not working in the PR. |
Roaders
force-pushed
the
esm-modules-fix
branch
3 times, most recently
from
e9f62f0 to
6ae93bb
Compare
|
|
|
this pr now switches all of the testing over to vitest. It seems much faster to me, is a much more "standard" testing setup and works with esm. jest has been completely removed and nyc has been removed from most places. It is still used in the root package.json to generate a report. I guess we could probably do that another way but I'm not sure what it's doing so I didn't want to touch that. |
packages/fdc3-agent-proxy/test/step-definitions/generic.steps.ts
Outdated
Show resolved
Hide resolved
packages/fdc3-get-agent/test/step-definitions/desktop-agent-api.steps.ts
Show resolved
Hide resolved
Roaders
force-pushed
the
esm-modules-fix
branch
3 times, most recently
from
e0cec0b to
010e6df
Compare
Roaders
force-pushed
the
esm-modules-fix
branch
from
010e6df to
ded154f
Compare
|
|
|
woohoo! build passing! 😄 I don't know why the static code analysis if failing or how to fix it... Rebasing this was a pain - took more than an entire morning. It would be good if we could merge this into main in the not too distant future to avoid any more merges or rebasing. I know this is a fairly major change though so it's going to need agreement from everyone. |
|
still no coverage being reported..... 😞 |
|
|
Roaders
force-pushed
the
esm-modules-fix
branch
from
10c0d74 to
1e05e02
Compare
|
|
|
|
|
looks like coverage is now fixed 😄 |
Roaders
force-pushed
the
esm-modules-fix
branch
from
93b8c6a to
237d25c
Compare
|
|
|
|
|
fixed the static code analysis by ignoring a testing step. I assume that we're ok with this? Feel free to revert. |
julianna-ciq
left a comment
julianna-ciq
left a comment
There was a problem hiding this comment.
Note that I have not tested this code, just reviewed it. I have a couple of sanity check questions for @Roaders , but as long as:
- The tests still pass
- ESM consumers can still use the built files
Then it lgtm
| "@finos/fdc3-standard": "2.2.2-beta.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@cucumber/cucumber": "10.3.1", |
There was a problem hiding this comment.
Just confirming - was this left in intentionally? I see that the other @cucumber dependencies were removed, as was the cucumber.yml file.
There was a problem hiding this comment.
no, this was unintentional. I will remove. Thanks for spotting.
There was a problem hiding this comment.
correction: no we are still using a type from @cucumber in some of the steps. Datatable. I had a very quick look to see if we could easily remove it and it doesn't seem so. It's a shame to have a hangover dependency because of one type but I think we'll have to live with this for now.
packages/fdc3-get-agent/package.json
Outdated
| "test": "tsc && vitest run", | ||
| "test:watch": "vitest", | ||
| "test:coverage": "vitest run --coverage", | ||
| "clean": "rimraf dist cucumber-report.html coverage node_modules test-results.xml", |
There was a problem hiding this comment.
Does vitest generate a cucumber-report.html file? Just want to make sure that if we're cleaning the file, it's being created somewhere. I only saw test-results.xml be created, maybe there's a report step somewhere?
There was a problem hiding this comment.
I am pretty sure that this can be removed as well. I'll double check.
There was a problem hiding this comment.
tests re-ran and no cucumber-report generated. removed reference
many thanks for the review @julianna-ciq . Yes the tests still pass (and reported in this pr) and the esm consumers can now consume the files - they were unable to before. |
|
|
|
|
5 participants
Describe your change
This is an initial attempt to fis the module formats of the libraries published from this repo. I think that the current issues with the published packages are:
module- this means that node does not know that it should treat the files as mjs rather than cjsNodeNextwhich means that we do not specify extensions on our imports and esm consuming apps will not be able to consume the files (unless bundler magic deals with this which I think webpack and angular build systems do)To fix these issues this PR:
"type": "module"Related Issue
fixes #1609
Contributor License Agreement
Review Checklist
DesktopAgent,Channel,PrivateChannel,Listener,Bridging)?JSDoc comments on interfaces and types should be matched to the main documentation in /docs
Conformance test definitions should cover all required aspects of an FDC3 Desktop Agent implementation, which are usually marked with a MUST keyword, and optional features (SHOULD or MAY) where the format of those features is defined
The Web Connection protocol and Desktop Agent Communication Protocol schemas must be able to support all necessary aspects of the Desktop Agent API, while Bridging must support those aspects necessary for Desktop Agents to communicate with each other
npm run build) run and the results checked in?Generated code will be found at
/src/api/BrowserTypes.tsand/or/src/bridging/BridgingTypes.tsBaseContextschema applied viaallOf(as it is in existing types)?titleanddescriptionprovided for all properties defined in the schema?npm run build) run and the results checked in?Generated code will be found at
/src/context/ContextTypes.tsTHIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE FINOS CORPORATE CONTRIBUTOR LICENSE AGREEMENT.
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.