Skip to content
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

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

DraTeots
Copy link
Collaborator

  • Works for updated angular
  • Uses the standalone controls recommended since Angular 17+

Copy link

@tmadlener tmadlener left a 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?

Comment on lines +193 to +194
- [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)

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

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)."

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.

Copy link
Collaborator

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.

@DraTeots
Copy link
Collaborator Author

About this problem:

....
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.

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.

@tmadlener
Copy link

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"

@DraTeots
Copy link
Collaborator Author

DraTeots commented Jul 18, 2024

✘ [ERROR] Could not resolve "jszip"

I believe it is to be a phoenix bug as I see jszip as phoenix-event-display dependency, but it is not listed in phoenix-ng-components dependencies, but the components use jszip. So it looks, that yarn here behave more responsible towards highlighting a possible error. Please, file a bug report with this.

P.S. At the same time, personally I prefer npm as in 2024 it fails less than yarn =p.

@EdwardMoyse EdwardMoyse merged commit 54e6178 into HSF:main Jul 29, 2024
1 check passed
@EdwardMoyse
Copy link
Collaborator

Sorry for the delay! And BTW I have no particular preference for yarn vs npm @DraTeots. I'm happy to discuss changing, if you think this is worthwhile (I honestly do not have enough experience to make the call).

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.

3 participants