Remove Hoisting #52

FrankEssenberger · 2020-03-20T15:05:39Z

Context

The hoisting from lerna caused problems with changes done to the package-lock.json. In this PR I tried to remove the hoisting. Besides a few minor things (jest needs a tsconfig and noch is not multi module enabled) it work quiete well.

I also needed to adjust some dependencies. I also "fixed" the minimist audit error. I added the minmist in version 1.2.5 to the root dependencies and removed adjusted the package-lock.json by removing the minimist in the optmist dependecy used by handlebar. This removed the audit error. Run npm audit with or without the entry to reproduce the issue.

Today a fix was merged: handlebars-lang/handlebars.js#1658
So we should check if this hack is still necessary once this gets published. For doing it one has to remove minimist and rerun npm i to regenerate the package lock.

General:

In the investigation I found that yarn provides more tools for fixing such issues. Especially if we can get rid of lerne by using it.
I am not sure about the package-lock.json on the module level. These were generated but do we need them? I simply do not know lerna well enough.

…ilities # Conflicts: # package-lock.json

…ilities # Conflicts: # test-packages/test-services/package.json

marikaner

I have a few questions in the comments. Also, do we need the package-lock files for sub packages?

marikaner · 2020-03-23T09:59:15Z

package.json

@@ -21,16 +21,14 @@
    "lint": "npx eslint --ext .ts .",
    "lint:fix": "npm run lint -- --fix",
    "lint:fix-cached": "npm run lint:fix -- --cache",
-    "postinstall": "npx lerna bootstrap --hoist --force-local --strict",
+    "postinstall": "npx lerna bootstrap  --strict",


There is one space too much ;)

Was --force-local causing issues too?

no put it back

marikaner · 2020-03-23T10:03:04Z

packages/core/test/test-util/destination-service-mocks.ts

+  accessToken: string,
+  uri: string = destinationServiceUri
+) {
+  return abc(uri, {


abc is not the best of names. What is it?

was a temp name renamed after the draft stage was left.

marikaner · 2020-03-23T10:04:19Z

packages/core/test/scp-cf/destination-accessor.spec.ts

-        mockInstanceDestinationsCall([], 200, providerServiceToken),
-        mockSubaccountDestinationsCall([], 200, providerServiceToken),
-        mockSingleDestinationCall(oauthSingleResponse, 200, destinationName, userApprovedSubscriberServiceToken)
+        mockInstanceDestinationsCall(nock, [], 200, subscriberServiceToken),


Why was this change necessary. It seems to me like a sensible default...

if the nock is imported from various node modules it is failing when used from different modules. There is something static involved. That is why I pass the nock imported in this test to the method located in a different one.

Huh. Can you share more details here? Is that related to a version upgrade? I never noticed this problem before.

No it is not related to an update. It is related to the previous hoisting. Situation before:
root

|---node_modules | |---nock |---package_1 |---node_modules |---ref_to_nock_root |---package_2 |---node_modules |---ref_to_nock_root

So when you did a require(nock) in package_1 or package_2 it was taking the same sources. Withtout the hoisting this has changed to:
root

|---package_1 |---node_modules |---nock |---package_2 |---node_modules |---nock

So when you do a require(nock) in package_1 for example in the destination-service-mocks.ts of core/test-util and then in the request-builder.spec.ts of the test-packages/integration-tests you load different source files. In principle this should not make a difference, but I found that there must be some encapsulation involved which breaks the matching on the first loaded nock. In other words the method nock.activeMocks() in package_1 do not show active mocks created by the second required nock in the package_2 and when calls are executed the mocks are not triggered.

Thats is why I pass the reference to the imported nock to the methods in the other packages.

Wow, that's evil... With that context this change makes a lot more sense.

But couldn't that be solved by only having nock on the root level? According to (my understanding of) the node resolution algorithm, the root level node_module would be used and it would always be the same.

technically yes, but I'm not a fan. You're essentially coupling the package to the project root. I am afraid that this a decision that has consequences that are not immediately obvious. Also it's something that you need to know when working with the project. If someone who's new to the project tries to run tests, then gets an error at runtime because nock isn't found, then looks at the package json and sees that nock isn't even listed as dependency and then adds it, I wouldn't blame them.

I already discussed with Marika on this topic and I have no strong opinions. In principle it would be strict to have only linting etc. things on root and all package related stuff on the sub-packages. However this is also kind of inconvinient because something is needed everywhere so why putting it N times?

My real Problem overall is: It should be consistent. Either put everything on root, or only the minimal stuff. Which one we chose I do not really care.

I am in favor of putting it in the root except for dependencies that are run from the command line (there it has to be in the same context). I think dragging the dependency through the tests is worse than someone running npm install in a subpackage only. That can be covered by documentation and reviews.
I am also ok with doing this one we move to yarn.

marikaner · 2020-03-23T10:05:54Z

test-packages/integration-tests/package.json

    "@types/jest": "^25.1.4",
+    "jsonwebtoken": "^8.5.1",
+    "bignumber.js": "8.1.1",
+    "nock": "^12.0.2",


Is this really necessary? As far as I understand if this is available on root level we don't need to add it to the packages, except for executables.

I tried to remove it and got compilation errors.

marikaner · 2020-03-23T10:19:52Z

test-packages/integration-tests/tsconfig.json

@@ -0,0 +1,8 @@
+{
+  "extends": "../../tsconfig.json",


Is this needed? Or is it just a left-over from trying things out?

it is necessary. It turns out that ts-jest needs a tsconfig.json implcitly.

marikaner · 2020-03-23T10:20:23Z

test-packages/test-services/package.json

@@ -16,6 +16,8 @@
  },
  "license": "Apache-2.0",
  "dependencies": {
-    "@sap-cloud-sdk/core": "^1.18.1"
+    "@sap-cloud-sdk/core": "^1.18.1",


Is this necessary? (Same as above)

I tried to remove it and got compilation errors.

…ilities # Conflicts: # package-lock.json # package.json # packages/core/test/scp-cf/destination-accessor.spec.ts # packages/core/test/test-util/destination-service-mocks.ts # test-packages/integration-tests/package.json

…ilities # Conflicts: # package-lock.json # test-packages/integration-tests/package.json

FrankEssenberger · 2020-04-02T14:44:47Z

Update: Hadlebars is updated with version 4.7.4 to not use the old minimist. So a npm audit fix was able to resolve the issue.

…ilities # Conflicts: # package-lock.json # test-packages/integration-tests/package.json

marikaner

I now tried this locally, npm i takes forever...

FrankEssenberger · 2020-04-06T07:14:09Z

I just made a test. I removed the all node_modules folders and then the whole npm i took around 50 secs including the compile steps:

added 948 packages from 764 contributors and audited 42165 packages in 50.488s

If I use npm ci it is a bit less:

added 948 packages in 45.761s

In the VPN it was around 10sec longer. On master using the hoisting it took my machine

added 969 packages from 773 contributors and audited 42060 packages in 61.443s

but it was already hot from the previous build so I guess the effect of heating up is more than the additional download time? In the pipeline I see times between 50 and 70 sec for the build with hoisting.

So it seems like the times are not really increased by the change. I do not know, perhaps also other colleagues can try it out.

…ilities # Conflicts: # package-lock.json # package.json

…ilities # Conflicts: # package-lock.json # package.json # test-packages/integration-tests/package.json

…ilities # Conflicts: # test-packages/test-services/package.json

marikaner

I am not very happy with the nock thingy, other than that I think this looks fine

marikaner · 2020-04-07T11:00:11Z

packages/core/test/scp-cf/destination-accessor.spec.ts

-        mockInstanceDestinationsCall([], 200, providerServiceToken),
-        mockSubaccountDestinationsCall([], 200, providerServiceToken),
-        mockSingleDestinationCall(oauthSingleResponse, 200, destinationName, userApprovedSubscriberServiceToken)
+        mockInstanceDestinationsCall(nock, [], 200, subscriberServiceToken),


I am in favor of putting it in the root except for dependencies that are run from the command line (there it has to be in the same context). I think dragging the dependency through the tests is worse than someone running npm install in a subpackage only. That can be covered by documentation and reviews.
I am also ok with doing this one we move to yarn.

* Issue #1000 fix, logger showing undefinedon Error objects. * Issue #1027, allow setting log level globally * Reordering priorities of level setter. Adding unit test for Error object and additional tests for logger levels. * Proper cleanup on logger level at the end of each test. Allowing falsy parameter at setLogLevel * Fixing some of the documentation comments * feat: Add custom query parameters option (#74) * chore: Automate generation of documentation and changelog (#72) * Improve generate docs * Add after bump script * Improve changelog * Add after bump script * Add change log to draft * Fix linting * Add npx * Add scripts * remove dash * Try as before * Add tests * Y u no work? * Fix typo * More output * Limits? * Try limit * Clean up * Add get-changelog action * Fix cases * Fix order * Just echo? * Fix typo * Cleanup * Gix test services script * Add latest version * Fix Re-export issue * Fix reexport for 1.18.1 * Fix docs for 1.18.0 * Also update DOCUMENTATION.md * Add docs index * Update package lock * Remove duplicate redirect * Update changelog * v1.19.0 * Revert "v1.19.0" This reverts commit e41efe7. * chore: Add changed files to staging area * v1.19.0 * Revert "v1.19.0" This reverts commit 0278a98. * Revert "chore: Add changed files to staging area" This reverts commit 30d499c. * chore: Add files to staging after version * v1.19.0 * Revert "v1.19.0" This reverts commit e2616a6. * chore: Fix typo in build.yml * v1.19.0 * chore: Update changelog * Remove Hoisting (#52) * release date and blog post (#76) * More tests in destination accessor (#75) Co-authored-by: Marika Marszalkowski <marika.marszalkowski@sap.com> Co-authored-by: Frank Essenberger <56544999+FrankEssenberger@users.noreply.github.com> Co-authored-by: Jordan Dukadinov <jdukadinov@gmail.com>

FrankEssenberger added 8 commits March 19, 2020 15:20

switch to marikas branch

9345b47

Merge remote-tracking branch 'origin/master' into Fix_handle_vulnerab…

45eeba3

…ilities # Conflicts: # package-lock.json

switch to master

6cd00b0

Merge remote-tracking branch 'origin/master' into Fix_handle_vulnerab…

5451619

…ilities # Conflicts: # test-packages/test-services/package.json

try build remote

5f02428

add npmrc

79c6c05

add npmrc to generator

89cc9e5

swtich to master

70e8577

marikaner requested changes Mar 23, 2020

View reviewed changes

FrankEssenberger mentioned this pull request Mar 23, 2020

Update Typescript #55

Merged

FrankEssenberger added 4 commits April 1, 2020 14:41

try with replaced minimist

cb16101

remove minimist from optimist in package-lock.json

75c818c

minimize used depedencies

ac30ae0

FrankEssenberger marked this pull request as ready for review April 1, 2020 15:52

FrankEssenberger added please merge labels Apr 1, 2020

FrankEssenberger added 2 commits April 1, 2020 17:54

Merge remote-tracking branch 'origin/master' into Fix_handle_vulnerab…

2134fec

…ilities # Conflicts: # package-lock.json # test-packages/integration-tests/package.json

merge master

df154ba

FrankEssenberger requested review from jjtang1985, mr-flannery and marikaner April 1, 2020 15:58

use npm audit to fix minimist issue

ea1e1f7

FrankEssenberger added 2 commits April 3, 2020 13:25

Merge remote-tracking branch 'origin/master' into Fix_handle_vulnerab…

dbc4642

…ilities # Conflicts: # package-lock.json # test-packages/integration-tests/package.json

merge master

1f78eeb

marikaner reviewed Apr 3, 2020

View reviewed changes

FrankEssenberger added 2 commits April 6, 2020 09:15

Merge remote-tracking branch 'origin/master' into Fix_handle_vulnerab…

135f2df

…ilities # Conflicts: # package-lock.json # package.json

update package lock

0e472c1

FrankEssenberger added 5 commits April 7, 2020 10:42

Merge remote-tracking branch 'origin/master' into Fix_handle_vulnerab…

270acd8

…ilities # Conflicts: # package-lock.json # package.json # test-packages/integration-tests/package.json

Merge remote-tracking branch 'origin/master' into Fix_handle_vulnerab…

cc4fb5a

…ilities # Conflicts: # test-packages/test-services/package.json

spy on used axios

7fbfb8b

linter

05360d3

fix test

da4efdb

marikaner approved these changes Apr 7, 2020

View reviewed changes

FrankEssenberger merged commit 38e94f4 into master Apr 7, 2020

FrankEssenberger deleted the Fix_handle_vulnerabilities branch April 7, 2020 11:27

marikaner pushed a commit to denidaja/cloud-sdk that referenced this pull request Apr 14, 2020

Remove Hoisting (SAP#52)

8fc138c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Hoisting #52

Remove Hoisting #52

FrankEssenberger commented Mar 20, 2020 •

edited

marikaner left a comment

marikaner Mar 23, 2020

marikaner Mar 23, 2020

FrankEssenberger Apr 1, 2020

marikaner Mar 23, 2020

FrankEssenberger Apr 1, 2020

marikaner Mar 23, 2020

FrankEssenberger Apr 2, 2020

mr-flannery Apr 2, 2020

FrankEssenberger Apr 3, 2020 •

edited

mr-flannery Apr 3, 2020

marikaner Apr 3, 2020

mr-flannery Apr 3, 2020

FrankEssenberger Apr 3, 2020

marikaner Apr 7, 2020

marikaner Mar 23, 2020

FrankEssenberger Apr 1, 2020

marikaner Mar 23, 2020

FrankEssenberger Apr 1, 2020

marikaner Mar 23, 2020

FrankEssenberger Apr 1, 2020

FrankEssenberger commented Apr 2, 2020

marikaner left a comment

FrankEssenberger commented Apr 6, 2020

marikaner left a comment

marikaner Apr 7, 2020

Remove Hoisting #52

Remove Hoisting #52

Conversation

FrankEssenberger commented Mar 20, 2020 • edited

Context

marikaner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankEssenberger Apr 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankEssenberger commented Apr 2, 2020

marikaner left a comment

Choose a reason for hiding this comment

FrankEssenberger commented Apr 6, 2020

marikaner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankEssenberger commented Mar 20, 2020 •

edited

FrankEssenberger Apr 3, 2020 •

edited