Skip to content

Conversation

@d-perl
Copy link
Contributor

@d-perl d-perl commented Nov 7, 2024

I'd like to start using this for some of our ongoing web GUI work (to hopefully take advantage of your upcoming PVWS integration), and not start off with out-of-date dependencies, so I took the liberty of upgrading them. It only required very minimal changes.

Copy link
Collaborator

@abigailalexander abigailalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I noticed a few build issues. As well as missing dependencies, the newer rollup doesn't quite work with the existing config. I received this error when trying to build:
Path of Typescript compiler option 'declarationDir' must be located inside the same directory as the Rollup 'file' option.
I did a quick investigation and it seems like this could be solved by changing declarationDir in tsconfig.json to be "dist", and then updating the output directory for the build file to match.
e.g. in package.json, main becomes "dist/index.cjs.js" instead of "dist/cjs/index.js" and likewise for module.

It would definitely be good to move cs-web-lib to react 18 so we can keep up to date.

Comment on lines 61 to 62
"rollup-plugin-peer-deps-external": "^2.2.4",
"rollup-plugin-postcss": "^4.0.2",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When attempting to build with npm run rollup, I received errors that these packages are missing. They're used in rollup.config.js.

@d-perl
Copy link
Contributor Author

d-perl commented Nov 14, 2024

Thanks for the review, I didn't notice those issues when building locally, perhaps I still had those settings in my local environment or something - I will look into them and try to add them to the CI too (seeing as that didn't break due to this issue...)

@abigailalexander
Copy link
Collaborator

That would be good, thanks. It seems like we only test the build in CI on tag push (when we publish to npm). It would be useful to have that as a check on a normal PR

@d-perl d-perl force-pushed the update_to_react_18_mui_6 branch from 3e55955 to 1e62764 Compare November 15, 2024 12:17
@d-perl
Copy link
Contributor Author

d-perl commented Nov 15, 2024

I think that the rollup build is fixed now, but I'm not sure how to test it properly - is there a way to run the unit tests against the built module or something?

Copy link
Collaborator

@abigailalexander abigailalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if there's an easy way to test a build other than importing the built package into a project and using it (which is what I do, with npm run rollup -> npm pack -> then in the new project I npm install the generated .tgz file).

I did this testing with a project locally and encountered errors:


    node_modules/@diamondlightsource/cs-web-lib/dist/esm/index.js:7:8:
      7 │ declare class DTime {
        │         ~~~~~
        ╵         ;

/node_modules/esbuild/lib/main.js:1472
  let error = new Error(text);
              ^

Looking at the dist/cjs/index.js and dist/esm/index.js files generated by the build, you can see their content is wrong. They're type declaration files instead of javascript files. I've pinpointed where it seems that this is coming from.

Other than that it looks good, thanks for adding the build into the CI.

Copy link
Collaborator

@abigailalexander abigailalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, all works now. I've merged the PVWS branch into main so it looks like there's a small merge conflict that appeared, but once that's resolved this can be merged in too

@d-perl d-perl force-pushed the update_to_react_18_mui_6 branch from c22772a to 78a2c81 Compare November 21, 2024 15:45
@abigailalexander abigailalexander merged commit 81b35a4 into DiamondLightSource:master Nov 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants