Skip to content
This repository has been archived by the owner on Nov 22, 2024. It is now read-only.

feat(socket-engine): add providers parameter #1072

Merged
merged 3 commits into from
Sep 29, 2018

Conversation

someApprentice
Copy link
Contributor

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@CaerusKaru CaerusKaru self-assigned this Sep 24, 2018
@CaerusKaru CaerusKaru added action: cleanup target: minor target: minor This PR is targeted for the next minor release comp: socket-engine labels Sep 24, 2018
@CaerusKaru CaerusKaru added this to the 6.0 milestone Sep 24, 2018
@CaerusKaru CaerusKaru changed the title Added providers injector feat(socket-engine): add providers parameter Sep 24, 2018
@CaerusKaru
Copy link
Member

Please sign the CLA, fix the build errors, and add a small unit test. Other than that, great work!

@someApprentice
Copy link
Contributor Author

Please sign the CLA, fix the build errors, and add a small unit test. Other than that, great work!

Affirmative! I will do that tomorrow, since time in my timezone is quite late.

@someApprentice
Copy link
Contributor Author

@CaerusKaru

Im pretty new at JavaScript and Angular itself and I have some issues with a building application.

After npm install and /universal$ bazel build -- :all I did bazel run @yarn//:yarn and /universal$ ./scripts/build-modules-dist.sh as it was on CircleCI, and I got this error:

ERROR: /universal/modules/common/BUILD.bazel:19:1: ng_package: Rollup //modules/common:npm_package failed (Exit 1)
[!] Error: 'RenderOptions' is not exported by ../../../../../execroot/nguniversal/bazel-out/k8-fastbuild/bin/modules/common/npm_package.es6/modules/common/engine/src/index.js
https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module
../../../../../execroot/nguniversal/bazel-out/k8-fastbuild/bin/modules/common/npm_package.es6/modules/common/engine/private_api.js (12:67)

Followed by suggested link I checked these files and as expected I didn't found exported RenderOptions in ...src/index.js:

export { } from "./interfaces";

...src/interfaces.js:

export function RenderOptions() { }
function RenderOptions_tsickle_Closure_declarations() {
    /** @type {?} */
    RenderOptions.prototype.bootstrap;
    /** @type {?|undefined} */
    RenderOptions.prototype.providers;
    /** @type {?|undefined} */
    RenderOptions.prototype.url;
    /** @type {?|undefined} */
    RenderOptions.prototype.document;
    /** @type {?|undefined} */
    RenderOptions.prototype.documentFilePath;
}

Can you give some tips what am I doing wrong? Should I simple add this dependency to the export?

@someApprentice
Copy link
Contributor Author

Can you give some tips what am I doing wrong? Should I simple add this dependency to the export?

I fixed this issue simple removed RenderOptions as ɵRenderOptions dependency from universal/modules/common/engine/private_api.ts, rebuild application (got error about lacking interface), returned this dependency back and rebuild application again. That time error didn't happen. Not sure what caused that behavior at the first time. Hope that helps someone in the future.

@someApprentice
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot
Copy link

CLAs look good, thanks!

@someApprentice
Copy link
Contributor Author

someApprentice commented Sep 29, 2018

ci/circleci: integration_test — Your tests failed on CircleCI

error "/home/circleci/ng/node_modules/@angular/animations" doesn't exist.

Not sure what causing that error on CircleCi. On my local machine the test is passed. Please check out your CircleCi environment and give me admonition if I though made a mistake.

universal$ ./integration/run_tests.sh
##################################
scripts/build-modules-dist.sh:
  building @nguniversal/* npm packages
##################################
WARNING: Switched off --watchfs again... temporarily falling back to manually checking files for changes
INFO: Analysed 7 targets (0 packages loaded).
INFO: Found 7 targets...
INFO: Elapsed time: 5.176s, Critical Path: 0.57s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
# Copy artifacts to /home/user/Downloads/universal/scripts/../dist/modules-dist/aspnetcore-engine
# Copy artifacts to /home/user/Downloads/universal/scripts/../dist/modules-dist/common
# Copy artifacts to /home/user/Downloads/universal/scripts/../dist/modules-dist/express-engine
# Copy artifacts to /home/user/Downloads/universal/scripts/../dist/modules-dist/hapi-engine
# Copy artifacts to /home/user/Downloads/universal/scripts/../dist/modules-dist/module-map-ngfactory-loader
# Copy artifacts to /home/user/Downloads/universal/scripts/../dist/modules-dist/socket-engine
#################################
Running integration test common
#################################
yarn install v1.10.1
info No lockfile found.
[1/4] Resolving packages...
warning [email protected]: 🙌  Thanks for using Babel: we recommend using babel-preset-env now: please read babeljs.io/env to update! 
[2/4] Fetching packages...
info [email protected]: The platform "linux" is incompatible with this module.
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
[4/4] Building fresh packages...
success Saved lockfile.
$ webdriver-manager update --gecko false --standalone false $CHROMEDRIVER_VERSION_ARG
[19:27:07] I/file_manager - creating folder /home/user/Downloads/universal/integration/common/node_modules/webdriver-manager/selenium
[19:27:07] I/config_source - curl -o/home/user/Downloads/universal/integration/common/node_modules/webdriver-manager/selenium/chrome-response.xml https://chromedriver.storage.googleapis.com/
[19:27:09] I/downloader - curl -o/home/user/Downloads/universal/integration/common/node_modules/webdriver-manager/selenium/chromedriver_2.42.zip https://chromedriver.storage.googleapis.com/2.42/chromedriver_linux64.zip
[19:27:10] I/update - chromedriver: unzipping chromedriver_2.42.zip
[19:27:10] I/update - chromedriver: setting permissions to 0755 for /home/user/Downloads/universal/integration/common/node_modules/webdriver-manager/selenium/chromedriver_2.42
Done in 69.82s.

...

Copy link
Member

@CaerusKaru CaerusKaru left a comment

Choose a reason for hiding this comment

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

LGTM

@CaerusKaru CaerusKaru added PR state: LGTM action: merge PR author is ready for this to merge and removed action: cleanup labels Sep 29, 2018
@CaerusKaru
Copy link
Member

The integration test job can be pretty flaky. I’ve been meaning to fix it, but in the meantime I’ve restarted the job (which usually works).

@CaerusKaru CaerusKaru merged commit c16860c into angular:master Sep 29, 2018
@CaerusKaru
Copy link
Member

@someApprentice Thank you again for putting this together, you did a great job!

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge PR author is ready for this to merge target: minor target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants