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 runtime dependency "uuid" #9077

Merged
merged 11 commits into from
May 27, 2020
Merged

Conversation

mikeharder
Copy link
Member

@mikeharder mikeharder commented May 21, 2020

@mikeharder
Copy link
Member Author

mikeharder commented May 26, 2020

@chradek, @jeremymeng: This dependency update is causing a build break in core-http. Can you please take a look?

src/util/utils.ts(4,20): error TS2307: Cannot find module 'uuid/v4'.

https://dev.azure.com/azure-sdk/public/_build/results?buildId=400159&view=logs&j=3dc8fd7e-4368-5a92-293e-d53cefc8c4b3&t=26452410-bb74-52b8-246c-3b6de8b4706f&l=225

The issue might be one of the known breaking changes:

https://github.com/uuidjs/uuid/blob/master/CHANGELOG.md#-breaking-changes

In general, we want to stay on the latest major of our depdendencies, but we could choose to stay on an older version of uuid if needed.

@chradek
Copy link
Contributor

chradek commented May 26, 2020

Ah, I've had to deal with this exact issue in another project. We should update our code to change how we import so we don't use the sub-module.

@mikeharder
Copy link
Member Author

Ah, I've had to deal with this exact issue in another project. We should update our code to change how we import so we don't use the sub-module.

Thanks. If you can make these changes with the existing version of uuid, then you can merge these changes first and I will rebase my PR. Otherwise you could add the changes onto my PR by pushing to my fork branch.

@chradek
Copy link
Contributor

chradek commented May 27, 2020

I've updated the affected packages.

@mikeharder mikeharder force-pushed the update-uuid branch 2 times, most recently from a156887 to 87a61ca Compare May 27, 2020 00:59
@mikeharder mikeharder self-assigned this May 27, 2020
@mikeharder mikeharder closed this May 27, 2020
@mikeharder mikeharder reopened this May 27, 2020
@mikeharder
Copy link
Member Author

/azp run js - core - ci

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@mikeharder
Copy link
Member Author

@chradek, @sadasant: The earlier fixes from @chradek unblocked the product build, but now test builds are failing in keyvault (two instances of this same error):

@azure/keyvault-secrets (1 minute 54.8 seconds)
>>> @azure/keyvault-secrets
npm run build:es6 && rollup -c rollup.test.config.js 2>&1
> @azure/keyvault-secrets@4.1.0-preview.2 build:es6 /home/vsts/work/1/s/sdk/keyvault/keyvault-secrets
> tsc -p tsconfig.json
dist-esm/test/*.test.js → dist-test/index.node.js...
(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
child_process (imported by ../../identity/identity/dist-esm/src/credentials/azureCliCredential.js)
stream (imported by ../../../common/temp/node_modules/.pnpm/registry.npmjs.org/jws/3.2.2/node_modules/jws/lib/sign-stream.js, ../../../common/temp/node_modules/.pnpm/registry.npmjs.org/jws/3.2.2/node_modules/jws/lib/verify-stream.js, ?stream?commonjs-external, ../../../common/temp/node_modules/.pnpm/registry.npmjs.org/jws/3.2.2/node_modules/jws/lib/data-stream.js, ../../../common/temp/node_modules/.pnpm/registry.npmjs.org/graceful-fs/4.2.4/node_modules/graceful-fs/legacy-streams.js)
util (imported by ../../../common/temp/node_modules/.pnpm/registry.npmjs.org/jws/3.2.2/node_modules/jws/lib/sign-stream.js, ../../../common/temp/node_modules/.pnpm/registry.npmjs.org/jws/3.2.2/node_modules/jws/lib/verify-stream.js, ?util?commonjs-external, ../../../common/temp/node_modules/.pnpm/registry.npmjs.org/jws/3.2.2/node_modules/jws/lib/data-stream.js, ../../../common/temp/node_modules/.pnpm/registry.npmjs.org/graceful-fs/4.2.4/node_modules/graceful-fs/graceful-fs.js, ../../../common/temp/node_modules/.pnpm/registry.npmjs.org/jwa/1.4.1/node_modules/jwa/index.js)
[...16 lines omitted...]
[!] Error: 'default' is not exported by ../../../common/temp/node_modules/.pnpm/registry.npmjs.org/uuid/8.1.0/node_modules/uuid/dist/esm-node/index.js, imported by ../../identity/identity/dist-esm/src/credentials/clientCertificateCredential.js
https://rollupjs.org/guide/en/#error-name-is-not-exported-by-module
../../identity/identity/dist-esm/src/credentials/clientCertificateCredential.js (6:7)
4: import qs from "qs";
5: import jws from "jws";
6: import uuid from "uuid";
          ^
7: import { readFileSync } from "fs";
8: import { createHash } from "crypto";
Error: 'default' is not exported by ../../../common/temp/node_modules/.pnpm/registry.npmjs.org/uuid/8.1.0/node_modules/uuid/dist/esm-node/index.js, imported by ../../identity/identity/dist-esm/src/credentials/clientCertificateCredential.js
    at error (/home/vsts/work/1/s/common/temp/node_modules/.pnpm/registry.npmjs.org/rollup/1.32.1/node_modules/rollup/dist/shared/node-entry.js:5400:30)
    at Module.error (/home/vsts/work/1/s/common/temp/node_modules/.pnpm/registry.npmjs.org/rollup/1.32.1/node_modules/rollup/dist/shared/node-entry.js:9824:16)
    at handleMissingExport (/home/vsts/work/1/s/common/temp/node_modules/.pnpm/registry.npmjs.org/rollup/1.32.1/node_modules/rollup/dist/shared/node-entry.js:9725:28)
    at Module.traceVariable (/home/vsts/work/1/s/common/temp/node_modules/.pnpm/registry.npmjs.org/rollup/1.32.1/node_modules/rollup/dist/shared/node-entry.js:10163:24)
    at ModuleScope.findVariable (/home/vsts/work/1/s/common/temp/node_modules/.pnpm/registry.npmjs.org/rollup/1.32.1/node_modules/rollup/dist/shared/node-entry.js:8770:39)
    at ChildScope.findVariable (/home/vsts/work/1/s/common/temp/node_modules/.pnpm/registry.npmjs.org/rollup/1.32.1/node_modules/rollup/dist/shared/node-entry.js:3065:38)
    at FunctionScope.findVariable (/home/vsts/work/1/s/common/temp/node_modules/.pnpm/registry.npmjs.org/rollup/1.32.1/node_modules/rollup/dist/shared/node-entry.js:3065:38)
    at ChildScope.findVariable (/home/vsts/work/1/s/common/temp/node_modules/.pnpm/registry.npmjs.org/rollup/1.32.1/node_modules/rollup/dist/shared/node-entry.js:3065:38)
    at FunctionScope.findVariable (/home/vsts/work/1/s/common/temp/node_modules/.pnpm/registry.npmjs.org/rollup/1.32.1/node_modules/rollup/dist/shared/node-entry.js:3065:38)
    at ChildScope.findVariable (/home/vsts/work/1/s/common/temp/node_modules/.pnpm/registry.npmjs.org/rollup/1.32.1/node_modules/rollup/dist/shared/node-entry.js:3065:38)

https://dev.azure.com/azure-sdk/public/_build/results?buildId=403148&view=logs&j=23e2e6de-b7c3-5918-7121-f16b46172e49&t=7bd26578-88b4-5baa-f548-7813777930ca&l=765

This might be the known breaking change "no more default export":

https://github.com/uuidjs/uuid/blob/master/CHANGELOG.md#-breaking-changes

@mikeharder mikeharder assigned sadasant and chradek and unassigned mikeharder May 27, 2020
@chradek
Copy link
Contributor

chradek commented May 27, 2020

I updated identity and a cosmos db sample. This time I did a more thorough search for all instances where uuid was being imported, rather than rely on rush build to report a failure.

Also ran rush build:test and see that keyvaults is building correctly now.

@mikeharder
Copy link
Member Author

/azp run js - eventhubs-client - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mikeharder
Copy link
Member Author

/azp run js - eventhubs-processor - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mikeharder
Copy link
Member Author

@xirzec, @bterlson: Do you approve this runtime dependency update?

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

:shipit:

@mikeharder mikeharder merged commit 53bdbe6 into Azure:master May 27, 2020
@mikeharder mikeharder deleted the update-uuid branch May 27, 2020 23:09
@dbarnespaychex
Copy link

Just FYI, the latest versions of UUID don't work on Node < 14. Unfortunately, the latest version of Node anyone can run under Azure is 12.14. Accordingly, the new "exports" configuration in UUID 8.x's package.json results in errors starting our Node 12 Azure containers.

You may either want to update your package.json's "engines" setting to reflect this new restriction, or consider downgrading the UUID version so it will work on older Node engines (like the latest version supported by Azure, Node v12.14).

In the meantime, we've had to hardcode our package.json @azure/event-hubs to "5.1.0", since "^5.1.0" will pick up this patch version update.

uuidjs/uuid#428 (comment)

@mikeharder
Copy link
Member Author

@dbarnespaychex: Thank you for reporting this. Can you please open an issue and include sample code and steps to reproduce the errors you are seeing?

@dbarnespaychex
Copy link

Issue #9515 opened. Thanks for your quick response. For anyone looking for a workaround, hard-code @azure/event-hubs to 5.1.0 in your package.json to get the older version of UUID.

@mikeharder
Copy link
Member Author

@dbarnespaychex: Thank you for the quick response as well. We are reviewing the issue right now and should have a reply by the end of the day.

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.

6 participants