-
Notifications
You must be signed in to change notification settings - Fork 59
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
Update of set-up-phoenix instructions for Angular 17+ #668
Conversation
DraTeots
commented
Jul 17, 2024
- Works for updated angular
- Uses the standalone controls recommended since Angular 17+
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.
Very nice thanks. I have effectively just one minor comment at the very end. Other than that I was able to follow these instructions easily and got to a working example. I have the following versions
$ node --version
v20.15.1
$ npm --version
10.7.0
$ ng version
_ _ ____ _ ___
/ \ _ __ __ _ _ _| | __ _ _ __ / ___| | |_ _|
/ △ \ | '_ \ / _` | | | | |/ _` | '__| | | | | | |
/ ___ \| | | | (_| | |_| | | (_| | | | |___| |___ | |
/_/ \_\_| |_|\__, |\__,_|_|\__,_|_| \____|_____|___|
|___/
Angular CLI: 17.3.8
Node: 20.15.1
Package Manager: npm 10.7.0
OS: linux x64
Angular: 17.3.12
... animations, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, router
Package Version
---------------------------------------------------------
@angular-devkit/architect 0.1703.8
@angular-devkit/build-angular 17.3.8
@angular-devkit/core 17.3.8
@angular-devkit/schematics 17.3.8
@angular/cli 17.3.8
@schematics/angular 17.3.8
rxjs 7.8.1
typescript 5.4.5
zone.js 0.14.8
One observation from me (which seems to be an issue in angular?), is that I get the following warning with npm start
. Everything seems to be working as expected though.
9:51:34 AM [vite] warning:
/home/tmadlener/work/students/desy_summer_2024/event-display-app/.angular/cache/17.3.8/vite/deps/chunk-7CYI36RD.js
2600| return import(
2601| /* webpackIgnore: true */
2602| module
| ^^^^^^
2603| );
2604| });
The above dynamic import cannot be analyzed by Vite.
See https://github.com/rollup/plugins/tree/master/packages/dynamic-import-vars#limitations for supported dynamic import formats. If this is intended to be left as-is, you can use the /* @vite-ignore */ comment inside the import() call to suppress this warning.
Plugin: vite:import-analysis
File: /home/tmadlener/work/students/desy_summer_2024/event-display-app/.angular/cache/17.3.8/vite/deps/chunk-7CYI36RD.js?v=7b1de038
9:51:34 AM [vite] warning:
/home/tmadlener/work/students/desy_summer_2024/event-display-app/.angular/cache/17.3.8/vite/deps/chunk-L6DXAODK.js
409| return import(
410| /* webpackIgnore: true */
411| "file://" + name
| ^^^^^^^^^^^^^^^^
412| );
413| }).finally(() => fs.unlinkSync(name));
The above dynamic import cannot be analyzed by Vite.
See https://github.com/rollup/plugins/tree/master/packages/dynamic-import-vars#limitations for supported dynamic import formats. If this is intended to be left as-is, you can use the /* @vite-ignore */ comment inside the import() call to suppress this warning.
Plugin: vite:import-analysis
File: /home/tmadlener/work/students/desy_summer_2024/event-display-app/.angular/cache/17.3.8/vite/deps/chunk-L6DXAODK.js?v=7b1de038
9:51:34 AM [vite] warning:
/home/tmadlener/work/students/desy_summer_2024/event-display-app/.angular/cache/17.3.8/vite/deps/chunk-L6DXAODK.js
461| return import(
462| /* webpackIgnore: true */
463| url
| ^^^
464| );
465| }
The above dynamic import cannot be analyzed by Vite.
See https://github.com/rollup/plugins/tree/master/packages/dynamic-import-vars#limitations for supported dynamic import formats. If this is intended to be left as-is, you can use the /* @vite-ignore */ comment inside the import() call to suppress this warning.
Another question related to npm vs yarn. phoenix itself seemed to have switched to yarn, and I have tried to also use yarn here instead of npm, i.e. the commands I ran, were
ng new event-display-app --style scss --routing true --interactive false
cd event-display-app
yarn set version berry # as recommended by main phoenix documentation
yarn add phoenix-ui-components
yarn add phoenix-event-display
ng generate component main-display
Then do all the necessary edits to the file, and then try to
yarn start
which will fail with the follwing:
✘ [ERROR] Could not resolve "jszip"
.yarn/__virtual__/phoenix-ui-components-virtual-7124f6bf67/5/.yarn/berry/cache/phoenix-ui-components-npm-2.16.0-baa536ee41-10c0.zip/node_modules/phoenix-ui-components/dist/fesm2022/phoenix-ui-components.mjs:40:18:
40 │ import JSZip from 'jszip';
╵ ~~~~~~~
The Yarn Plug'n'Play manifest forbids importing "jszip" here because it's not listed as a
dependency of this package:
.pnp.cjs:10935:31:
10935 │ "packageDependencies": [\
╵ ~~
You can mark the path "jszip" as external to exclude it from the bundle, which will remove this
error and leave the unresolved path in the bundle.
I suppose the issue is actually one from phoenix, and the fix is to simply "switch off" the Plug'n'Play by adding the following line to the .yarnrc.yml
file
nodeLinker: node-modules
(and then re-doing yarn install
)
Maybe this is worth mentioning as well in the Resolving problems section?
- [geometry examples](https://github.com/HSF/phoenix/tree/main/packages/phoenix-ng/projects/phoenix-app/src/assets/geometry) | ||
- [event examples](https://github.com/HSF/phoenix/tree/main/packages/phoenix-ng/projects/phoenix-app/src/assets/files) |
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.
From the current paths that are in the main-display.components.ts
file it's not entirely obvious what would be two example files here that lead to a consistent view in the end. If there is such a pair of paths / files, I think it would be nice to state them here, as it would then be more easily possible to check if things are working as expected.
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.
That, I believe, @EdwardMoyse could help
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.
Right, well someone rolling their own version of Phoenix is presumably going to have to add their own geometry and event data ... I wasn't really expecting that anyone would use what is in src/assets
. Maybe it's best just to add a caveat beneath these lines saying "(Obviously the geometry and event data should match, so if you use the TrackML event data, you should use the matching TrackML geometry)."
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.
Just for completeness: It's possible to get e.g. CMS easily loaded because it's one file. But IIRC example data only exists for ATLAS, but that is a much more "fragmented" detector. So in the end this would effectively mainly be the icing on the cake to make sure that the basic setup worked as expected. But I also understand that this might be more work than is worth the effort.
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.
ATLAS and CMS are very similar detectors, it's just that for the CMS demo it was fine just to have one detector with no ability to switch off/on sub detectors. But we already knew that ATLAS wanted to use Phoenix as a fully functional event display, and so this was not acceptable (and at the time Phoenix could only add separate menu items for separate geometry files). The other advantage of having lots of separate sub-detector geometries is it means we can use these to make the different versions of ATLAS (e.g. Run1, Run2, Run3, Run4) by just combining what remained from the previous run, and adding what was upgraded.
I guess I could pretty easily make a complete ATLAS detector though... and this might have the advantage of being substantially smaller in file size.... I will have a look.
About this problem:
TL;DR; Ignore it. This happens, because phoenix libraries depend on three.js and jszip which are so called "common js" libraries. Those libraries in turn depends on many other "common js" libraries. Those libraries sometimes has different ways to import things as more or less standardized importing system is still kind of new in js world (some libraries exist for decades). So Vite which is used under the hood of ng can't analyze the import and issue the warning. There are 2 ways to solve the warnings: use another libraries, which is considered to be not possible or to reconfigure angular. As far as I googled it is not obvious reconfiguration (i.e. not setting some flag to true) so for the sake of tutorial simplicity - ignore it. Or... please let me know if I am wrong here, and it is a matter of some flag. |
I found the same information as you I think about the vite issue, so maybe adding a sentence to the instructions would be helpful, so that other newcomers at least know that this is something that is known and "safe to ignore" |
I believe it is to be a phoenix bug as I see P.S. At the same time, personally I prefer npm as in 2024 it fails less than yarn =p. |
Sorry for the delay! And BTW I have no particular preference for |