refactor(stark-all): replace 'lodash' by tree-shakeable 'lodash-es'. Add '@types/lodash-es' to get correct typings. #1147

carlo-nomes · 2019-02-22T10:04:05Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/NationalBankBelgium/stark/blob/master/CONTRIBUTING.md#-commit-message-guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #150

What is the new behavior?

Lodash-es imports and typings

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Other information

coveralls · 2019-02-26T08:27:01Z

Coverage decreased (-2.0%) to 92.725% when pulling 3a8a21b on cnomes:feature/lodash-types into 517df61 on NationalBankBelgium:master.

christophercr

Small remarks

christophercr · 2019-02-26T16:06:48Z

packages/stark-core/karma.conf.js

+		...defaultKarmaConfig.karmaTypescriptConfig,
+		bundlerOptions: {
+			...defaultKarmaConfig.karmaTypescriptConfig.bundlerOptions,
+			transforms: [


Can you add these to the exports of this file so that you can access it in stark-core/karma.conf.ci.js instead of duplicating this block in these 2 files?

I was thinking of doing this with #1145 (amongst other things). But I could append a commit to this PR.

Mmm well I don't think that this will change that much when #1145 is solved right? We would need to apply the same logic here... so we could re-use it here instead of duplicating it

I was thinking of writing (using) a merge function so that a single config file could be exported from the karma.conf.js

packages/stark-ui/karma.conf.js

christophercr · 2019-02-26T16:10:37Z

packages/stark-ui/package.json

@@ -28,6 +28,7 @@
  "dependencies": {
    "@angular/material-moment-adapter": "^7.0.0",
    "@mdi/angular-material": "^3.3.92",
+    "@types/lodash-es": "^4.17.1",


Shouldn't the types from lodash-es be taken from stark-core by changing the path in the tsconfig.json of stark-ui ?

I will try again, but I think my IDE didn't recognize the types when I tried that. 🤔

I'm hitting a lot of other issues. Maybe this is more suitable for a separate ticket? 😕

showcase/package.json

starter/package.json

starter/package-lock.json

packages/stark-ui/karma.conf.js

carlo-nomes · 2019-03-22T08:07:59Z

The build works, so this Pr is ready for some extra reviewing. 😄

christophercr

Just small remarks... the rest looks good 👍

packages/stark-testing/karma.conf.ci.js

packages/stark-testing/karma.conf.js

packages/stark-testing/karma.conf.ci.js

packages/stark-testing/karma.conf.js

packages/stark-testing/karma.conf.ci.js

packages/stark-testing/karma.conf.js

packages/stark-ui/tsconfig-build.json

packages/stark-build/config/tslint.json

christophercr · 2019-03-22T09:52:33Z

Also these files need to be changed (they are still using lodash instead of lodash-es):

packages/stark-core/src/modules/http/services/http.service.ts
packages/stark-core/src/util/http.util.ts
packages/stark-ui/src/modules/date-picker/components/date-picker.component.ts
showcase/src/app/demo-ui/pages/generic-search/components/demo-generic-search-form.component.ts
showcase/src/app/demo-ui/pages/message-pane/demo-message-pane-page.component.ts
showcase/src/assets/examples/message-pane/message-pane.ts

- added used modules to rollup globals (TODO: NationalBankBelgium#1129) - blacklisted `lodash-es` (use submodules instead) - added `karma-typescript-es6-transform` for transforming es6 dependencies - added workaround for monounity/karma-typescript#320 in `base.spec.ts` (stark-core, stark-ui, showcase, starter) - aliased all used lodash modules in karma.conf.js (TODO NationalBankBelgium#1145) - minor refactors ISSUES CLOSED: NationalBankBelgium#150

This commit will refactor all used instances of `const (.*) = require("lodash/(.*)")` to `import (.*) from "lodash/(.*)"` ISSUES CLOSED: NationalBankBelgium#150

christophercr · 2019-03-27T15:57:16Z

LGTM 👍

@SuperITMan can you have a look on this PR?

…a CI config (bug introduced in NationalBankBelgium#1147)

carlo-nomes requested review from christophercr, SuperITMan and Mallikki February 22, 2019 10:04

carlo-nomes force-pushed the feature/lodash-types branch 4 times, most recently from 894bdaf to 44b2475 Compare February 26, 2019 08:19

christophercr suggested changes Feb 26, 2019

View reviewed changes

christophercr added this to the 10.0.0-beta.5 milestone Feb 26, 2019

christophercr added type: enhancement comp: stark-all To apply for all Stark modules labels Feb 26, 2019

christophercr changed the title ~~Feature/lodash types~~ refactor(stark-all): replace 'lodash' by tree-shakeable 'lodash-es'. Add '@types/lodash-es' to get correct typings. Feb 26, 2019

carlo-nomes force-pushed the feature/lodash-types branch 7 times, most recently from b88f813 to 0d3d8e0 Compare February 28, 2019 13:40

SuperITMan modified the milestones: 10.0.0-beta.5, 10.0.0-beta.6 Feb 28, 2019

carlo-nomes force-pushed the feature/lodash-types branch from 0d3d8e0 to 6e26513 Compare March 4, 2019 14:13

SuperITMan modified the milestones: 10.0.0-beta.6, 10.0.0-beta.7 Mar 5, 2019

carlo-nomes force-pushed the feature/lodash-types branch 2 times, most recently from edbad62 to 2a5a870 Compare March 21, 2019 16:44

christophercr suggested changes Mar 22, 2019

View reviewed changes

carlo-nomes force-pushed the feature/lodash-types branch 3 times, most recently from b4a5071 to f8ab02c Compare March 27, 2019 15:06

carlo-nomes added 2 commits March 27, 2019 16:16

refactor(stark-all): update lodash usage

852d995

This commit will refactor all used instances of `const (.*) = require("lodash/(.*)")` to `import (.*) from "lodash/(.*)"` ISSUES CLOSED: NationalBankBelgium#150

carlo-nomes force-pushed the feature/lodash-types branch from f8ab02c to 1ff90ff Compare March 27, 2019 15:17

ci(stark-all): fix package-lock.json files after lodash-es update

3a8a21b

carlo-nomes force-pushed the feature/lodash-types branch from 1ff90ff to 3a8a21b Compare March 27, 2019 15:21

christophercr approved these changes Mar 27, 2019

View reviewed changes

christophercr removed the request for review from Mallikki March 28, 2019 11:08

SuperITMan approved these changes Apr 1, 2019

View reviewed changes

SuperITMan merged commit c5c34c6 into NationalBankBelgium:master Apr 1, 2019

christophercr added a commit to christophercr/stark that referenced this pull request Apr 9, 2019

fix(stark-ui): fix wrong code coverage report generation path in karm…

8d656e6

…a CI config (bug introduced in NationalBankBelgium#1147)

christophercr mentioned this pull request Apr 9, 2019

feat(stark-all): upgrade to TypeScript 3.2 (latest version supported in Angular 7.2) #1242

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(stark-all): replace 'lodash' by tree-shakeable 'lodash-es'. Add '@types/lodash-es' to get correct typings. #1147

refactor(stark-all): replace 'lodash' by tree-shakeable 'lodash-es'. Add '@types/lodash-es' to get correct typings. #1147

carlo-nomes commented Feb 22, 2019

coveralls commented Feb 26, 2019 •

edited

Loading

christophercr left a comment

christophercr Feb 26, 2019

carlo-nomes Feb 27, 2019

christophercr Feb 27, 2019

carlo-nomes Feb 27, 2019

christophercr Feb 26, 2019

carlo-nomes Feb 27, 2019

carlo-nomes Feb 27, 2019

carlo-nomes commented Mar 22, 2019

christophercr left a comment

christophercr commented Mar 22, 2019

christophercr commented Mar 27, 2019

refactor(stark-all): replace 'lodash' by tree-shakeable 'lodash-es'. Add '@types/lodash-es' to get correct typings. #1147

refactor(stark-all): replace 'lodash' by tree-shakeable 'lodash-es'. Add '@types/lodash-es' to get correct typings. #1147

Conversation

carlo-nomes commented Feb 22, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

coveralls commented Feb 26, 2019 • edited Loading

christophercr left a comment

Choose a reason for hiding this comment

christophercr Feb 26, 2019

Choose a reason for hiding this comment

carlo-nomes Feb 27, 2019

Choose a reason for hiding this comment

christophercr Feb 27, 2019

Choose a reason for hiding this comment

carlo-nomes Feb 27, 2019

Choose a reason for hiding this comment

christophercr Feb 26, 2019

Choose a reason for hiding this comment

carlo-nomes Feb 27, 2019

Choose a reason for hiding this comment

carlo-nomes Feb 27, 2019

Choose a reason for hiding this comment

carlo-nomes commented Mar 22, 2019

christophercr left a comment

Choose a reason for hiding this comment

christophercr commented Mar 22, 2019

christophercr commented Mar 27, 2019

coveralls commented Feb 26, 2019 •

edited

Loading