-
Notifications
You must be signed in to change notification settings - Fork 405
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
test(integration): angular 5.2 + typescript 2.7 #852
Conversation
# Conflicts: # integrations/hello-world-ng5/.angular-cli.json # integrations/hello-world-ng5/package.json # integrations/hello-world-ng5/src/app/app.module.ts
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!
Just a note that, as discussed, Angular 5.2 does officially not support TS 2.7, but there is an existing issue #357 that has been around for a while that points out why we upgraded to TS2.7 as a workaround (at least for now). |
@splincode I recommend just adding a 'pre*' npm script step for the specific integration build task that copies the |
We can't support TypeScript 2.7, because my build does not work at all for the new version of NGXS (3.4.0-dev): ReactiveX/rxjs#4512 I propose to make an announcement where we indicate the versions that we can minimally support. Minimal TypeScript - 2.8 |
# Conflicts: # packages/hmr-plugin/src/public_api.ts # packages/hmr-plugin/src/symbols.ts # yarn.lock
integration test is ready |
NgxsHmrOptions, | ||
NgxsHmrSnapshot, | ||
WebpackModule, | ||
BootstrapModuleFn |
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 have WebpackModule
and BootstrapModuleFn
been added to the exports?
Was something not working? It works fine in my app without 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.
no, it works without, I just wanted to use the types in the example, the user may also want this
@@ -50,6 +42,6 @@ | |||
"protractor": "5.1.2", | |||
"ts-node": "4.1.0", | |||
"tslint": "5.9.1", | |||
"typescript": "2.7.1" | |||
"typescript": "2.8.1" |
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 was the reason for this version bump?
I had it working with 2.7.1
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 did the same thing, copied the directories, it did not work, I did it through the paths, but it did not work there
@markwhitfeld we need fix it |
I think we need to add unit tests for this integration test. |
Ok, I got the router working. Fixed the issue above. |
One last issue I have picked up... @splincode could you try to reproduce and see if there is a bug in HMR? ... maybe HMR doesn't support Angular 5? |
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.
HMR seems to not be working. See comment above.
@splincode will fix the hmr issue with NG5 in a separate PR. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
#847
What is the new behavior?
Does this PR introduce a breaking change?
Other information