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 to Angular 6 #30

Merged
merged 2 commits into from
Jul 21, 2018
Merged

Update to Angular 6 #30

merged 2 commits into from
Jul 21, 2018

Conversation

timdeschryver
Copy link
Contributor

No description provided.

@timdeschryver
Copy link
Contributor Author

The latest commit uses the angular cli to build the library.
Feedback is as always welcome, I'll walk through it again tomorrow.

"deleteDestPath": false,
"lib": {
"entryFile": "src/public_api.ts",
"styleIncludePaths": ["./src/scss"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to build the styles with the cli, but the styles didn't get picked up.
Any ideas? Or do we keep it as it is?

Copy link
Owner

Choose a reason for hiding this comment

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

I'll check out the PR later on my local machine and see if I find something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You should also add "styleIncludePaths": ["./src/scss"] to ng-package-prod.json(?) file because we're building it with the prod flag.

{
"extends": "../../tslint.json",
"rules": {
"directive-selector": [false, "attribute", "ngx", "camelCase"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This rule is disabled for now.
When we rename selectItem to something more suitable, we'll have to turn this back on.

@@ -29,6 +28,7 @@ import { FooterComponent } from './footer/footer.component';
import { HeaderComponent } from './header/header.component';
import { PhoneComponent } from './phone/phone.component';
import { LayoutModule } from '@angular/cdk/layout';
import { DragToSelectModule } from '../../projects/ngx-drag-to-select/src/lib/drag-to-select.module';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to import from ngx-drag-to-select here?
If so I think we'll have to modify the build process I think.

package.json Outdated
"build:server": "ng build --prod --project=server",
"build:lib": "npm run clean && npm run build:lib:code && npm run build:lib:sass && npm run copy:styles && npm run copy:readme",
"build:lib:code": "ng build --prod --project=ngx-drag-to-select",
"build:lib:sass": "node-sass projects/ngx-drag-to-select/src/scss/ngx-drag-to-select.scss -o dist/ngx-drag-to-select",
"deploy": "ngh --dir dist/browser",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this used @d3lm ?

Copy link
Owner

Choose a reason for hiding this comment

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

This is only meant to be used locally, if needed.

const mousePosition = getRelativeMousePosition(event, container);
// Explicit cast to `MousePosition` in order to "use" `MousePosition`
// we have to import `MousePosition` otherwise we'll get a build error
const mousePosition: MousePosition = getRelativeMousePosition(event, container);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you got a better solution?
The build was throwing an error because it didn't know MousePosition.

Copy link
Owner

Choose a reason for hiding this comment

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

And this doesn't work if we add the return type to getRelativeMousePosition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try it out later today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a quick check and it doesn't work.
I added a return type to getMousePosition because getRelativeMousePosition already has a return type?

Copy link
Owner

Choose a reason for hiding this comment

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

When I remove the type annotation and run either yarn start or build the app for prod with yarn build everything works for me. Can you try again? What is the error? Is TS complaining?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error comes when you try to build the lib build:lib.

BUILD ERROR
projects/ngx-drag-to-select/src/lib/operators.ts(6,14): error TS4023: Exported variable 'createSelectBox' has or is using name 'MousePosition' from external module "C:/Users/tdeschryver/dev/ngx-drag-to-select/projects/ngx-drag-to-select/src/lib/models" but cannot be named.

Error: projects/ngx-drag-to-select/src/lib/operators.ts(6,14): error TS4023: Exported variable 'createSelectBox' has or is using name 'MousePosition' from external module "C:/Users/tdeschryver/dev/ngx-drag-to-select/projects/ngx-drag-to-select/src/lib/models" but cannot be named.

    at Object.<anonymous> (C:\Users\tdeschryver\dev\ngx-drag-to-select\node_modules\ng-packagr\lib\ngc\compile-source-files.js:53:68)
    at Generator.next (<anonymous>)
    at fulfilled (C:\Users\tdeschryver\dev\ngx-drag-to-select\node_modules\ng-packagr\lib\ngc\compile-source-files.js:4:58)

projects/ngx-drag-to-select/src/lib/operators.ts(6,14): error TS4023: Exported variable 'createSelectBox' has or is using name 'MousePosition' from external module "C:/Users/tdeschryver/dev/ngx-drag-to-select/projects/ngx-drag-to-select/src/lib/models" but cannot be named.

Error: projects/ngx-drag-to-select/src/lib/operators.ts(6,14): error TS4023: Exported variable 'createSelectBox' has or is using name 'MousePosition' from external module "C:/Users/tdeschryver/dev/ngx-drag-to-select/projects/ngx-drag-to-select/src/lib/models" but cannot be named.

    at Object.<anonymous> (C:\Users\tdeschryver\dev\ngx-drag-to-select\node_modules\ng-packagr\lib\ngc\compile-source-files.js:53:68)
    at Generator.next (<anonymous>)
    at fulfilled (C:\Users\tdeschryver\dev\ngx-drag-to-select\node_modules\ng-packagr\lib\ngc\compile-source-files.js:4:58)

Copy link
Owner

Choose a reason for hiding this comment

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

Yep, indeed. This makes sense. The compiler is trying to figure out the shape of MouseEvent but it can't cause it's defined in a different module and never imported to the operators.ts module. So we can just add the type annotation and remove the comment I guess. What do you think? Or should we keep the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid future us will try to remove it 😅 and bumping into this problem again.

Copy link
Owner

@d3lm d3lm Jul 20, 2018

Choose a reason for hiding this comment

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

Haha you're right, we can leave it in then. Maybe let's reword this to Type annotation is required here, because 'getRelativeMousePosition' return a 'MousePosition' and the TS compiler cannot figure out the shape of this type., or something along those lines 🙂

@timdeschryver timdeschryver force-pushed the pr/angular6 branch 2 times, most recently from 9e8af8c to a07de70 Compare July 19, 2018 03:21
package.json Outdated
"test:ci": "npm run test && start-server-and-test start http-get://localhost:4200 e2e:ci",
"test:watch": "jest --watch",
"test:watch": "npm run test -- --watch",
"lint:check": "ng lint",
"lint:fix": "ng lint --fix",
Copy link
Owner

Choose a reason for hiding this comment

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

This command is broken with the latest CLI. It doesn't allow multiple apps for a single command I suppose. We have to change this to ng lint app --fix && ng lint ngx-drag-to-select --fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch, you're right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Owner

Choose a reason for hiding this comment

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

Do you need the --project flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope we don't! I also removed the --project flag in the build scripts.

Copy link
Owner

Choose a reason for hiding this comment

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

Yay 🎉

@timdeschryver timdeschryver force-pushed the pr/angular6 branch 3 times, most recently from 6366723 to 41d246f Compare July 19, 2018 08:21
@d3lm d3lm added this to the 2.0 milestone Jul 19, 2018
@d3lm d3lm merged commit fa010e2 into d3lm:master Jul 21, 2018
@d3lm
Copy link
Owner

d3lm commented Jul 21, 2018

Woho 🎉 We finally made it to Angular 6. Thanks so much @timdeschryver for all the work you put into this PR. I really appreciate your help.

@timdeschryver timdeschryver deleted the pr/angular6 branch July 21, 2018 12:53
@d3lm d3lm mentioned this pull request Jul 21, 2018
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.

None yet

2 participants