-
Notifications
You must be signed in to change notification settings - Fork 234
fix(skip-list): fixed the handling and passing of the skip list #902
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
base: main
Are you sure you want to change the base?
Conversation
asollberger
commented
Jul 22, 2025
- the skip list was ignored for secondary entry points, and thus libraries were brought back in (libs/native-federation-core/src/lib/config/share-utils.ts)
- fixed a few typos
- the skip list was ignored for secondary entry points, and thus libraries were brought back in (libs/native-federation-core/src/lib/config/share-utils.ts) - fixed a few typos
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.
I appreciate the cleanup but what are you trying to solve? I do agree that it is a bit cleaner but I do have some questions.
Could you take a look at that?
# Angular | ||
.angular | ||
|
||
.nx |
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.
Any reason why you removed this?
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.
I created the PR when npm had flagged a css framework package as a security issue and could not get the build to run. I will probably retry fresh and see. No other reason to remove the .nx, will remove the change again, been a bit too busy lately to take care of my two PR's.
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.
Great! thanks, that would be nice.
shareObject: SharedConfig, | ||
acc: Record<string, SharedConfig> | ||
acc: Record<string, SharedConfig>, | ||
preparedSkipList: PreparedSkipList |
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.
Seems like you're only adding the preparedSkipList but not using it?
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.
What about line 139?
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.
In line 139, you are giving the parameter to the same function, so yes it's included in the recursive loop, but not used in the function at all. Maybe it would make more sense to use the parameter in line 132? Or remove the parameter entirely since it's a bit useless now.
"version": "3.3.1", | ||
"license": "MIT" | ||
"license": "MIT", | ||
"dependencies": { |
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.
Any particular reason for adding these dependencies?
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.
I thought it would be helpful to declare them since they are used in it ...
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.
Fair play
"@angular/core": "^20.1.0", | ||
"@angular/platform-browser-dynamic": "^20.1.0" | ||
"@angular/platform-browser-dynamic": "^20.1.0", | ||
"@angular-devkit/build-angular": { |
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.
Why is this?
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.
This was the only way to upgrade jest to that version
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.
Would it be possible to revert this file as much as possible given the scope of the PR? Thanks
shareObject: SharedConfig, | ||
acc: Record<string, SharedConfig> | ||
acc: Record<string, SharedConfig>, | ||
preparedSkipList: PreparedSkipList |
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.
In line 139, you are giving the parameter to the same function, so yes it's included in the recursive loop, but not used in the function at all. Maybe it would make more sense to use the parameter in line 132? Or remove the parameter entirely since it's a bit useless now.
key, | ||
shareObject | ||
shareObject, | ||
preparedSkipList |
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.
I am starting to see what you're trying to achieve, great stuff!
I am wondering, would it make sense to remove PREPARED_DEFAULT_SKIP_LIST
completely after this?
"@angular/core": "^20.1.0", | ||
"@angular/platform-browser-dynamic": "^20.1.0" | ||
"@angular/platform-browser-dynamic": "^20.1.0", | ||
"@angular-devkit/build-angular": { |
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.
Would it be possible to revert this file as much as possible given the scope of the PR? Thanks
"version": "3.3.1", | ||
"license": "MIT" | ||
"license": "MIT", | ||
"dependencies": { |
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.
Fair play