-
Notifications
You must be signed in to change notification settings - Fork 12k
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
feat(@ngtools/webpack): replace server bootstrap code #5194
Conversation
@FrozenPandaz can you add a test that shows these changes work? A good place to add would be side by side with the other webpack plugin tests in https://github.com/angular/angular-cli/tree/master/tests/e2e/tests/packages/webpack These tests will create a new project from the files in https://github.com/angular/angular-cli/tree/master/tests/e2e/assets/webpack. You'll need a new project in the last folder, and a new file that tests it in the first. |
@filipesilva yes i will add tests, thanks for pointing me to the right place i was actually confused about that because theres a loader spec which looks quite empty. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
1c8ceaf
to
3490c18
Compare
CLAs look good, thanks! |
407fde0
to
d967b51
Compare
@filipesilva I've added tests. Please take a look. |
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.
Good job on the test setup! I'm requesting Hans's review on this as the authority on the plugin.
9304bb7
to
24a6914
Compare
I rebased this again. @hansl |
24a6914
to
f8aea74
Compare
83db000
to
854f960
Compare
Could you provide some updates about this issue ? |
Anything new regarding this PR? It is extremely important for our project. |
a4894bf
to
e61fae4
Compare
1ac4d48
to
497f5b1
Compare
CLAs look good, thanks! |
9a7716f
to
016445e
Compare
7aa110f
to
31774d7
Compare
@hansl This should be ready for you to take a look |
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.
LGTM, thanks!
feat(@ngtools/webpack): replace server bootstrap code (angular#5194)
feat(@ngtools/webpack): replace server bootstrap code (angular#5194)
Saw it merged to master, any ideas when we're releasing to npm? |
Is this depends on #5547? |
Nope this does not depend on #5547 |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently the loader is only replacing the bootstrap code of platformBrowser.
It should replace the bootstrap code of platformServer as well.
I also made it expandable to other platforms. We should probably add webworker platform as well.
I can do that in a separate pull request?
Motivation for this is better support of platform-server via @ngtools/webpack