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

Use latest (successfully building) version of uuid #3924

Merged
merged 3 commits into from
May 31, 2022

Conversation

jrencz
Copy link
Contributor

@jrencz jrencz commented Oct 21, 2021

Removes one of 2 warnings mentioned in #3812

The way uuid is used in

aws-sdk-js/lib/util.js

Lines 920 to 927 in bb861ce

/**
* @api private
*/
uuid: {
v4: function uuidV4() {
return require('uuid').v4();
}
},

was already compatible with latest version

Checklist
  • npm run test passes

On npm run test passes: masterbefore this change whennpm run unit` is run @ node 16.2:
2882 passing (18s)
7 failing

With this PR:
2882 passing (15s)
7 failing

On npm run integration run @ node 16.2 same 4 problems are raised with and without uuid version bump.
No test was fixed, no test was broken.

@BourgoisMickael
Copy link

BourgoisMickael commented Apr 26, 2022

@AllanZhengYP @trivikr Can you check this 1 line PR to update uuid, shouldn't be hard to merge

@davecarlson
Copy link

davecarlson commented May 12, 2022

@jrencz @BourgoisMickael - The CI build failed, you might want to take a look so we can get this merged in !

@jrencz jrencz force-pushed the chore/update-uuid branch from 523fbb7 to 4097fd9 Compare May 12, 2022 20:13
@jrencz jrencz requested a review from a team as a code owner May 12, 2022 20:13
@jrencz
Copy link
Contributor Author

jrencz commented May 12, 2022

@davecarlson I rebased to current master.

I tried to run npm run test locally, but all I can say is "different year, same story

4447 passing (22s)
7 failing

just that now there's twice as much tests, but some stubborn 7 keep failing locally on node 16
For some reason I'm not able to install node 12 (as expressed in buildspec.yml) on the computer I have (Mac with M1 chip)

Let's see what will it be this time on CodeBuild

@jrencz
Copy link
Contributor Author

jrencz commented May 12, 2022

@davecarlson t seems to fail on CodeBuild for different reasons than locally - there are no failed tests. There's failure during minification of code run by a rake task. And that's promissing

  task :build_all => [:setup_dist_tools, :dist_path] do
    sh({"MINIFY" => "1"}, "#{$BUILDER} all > dist/aws-sdk-all.js")
  end

I tried running that locally and indeed with 8.3.2 it fails.

I bisected versions down and it turns out the latest that passes that step is 8.0.0

Next one 8.1.0 fails.

There's a significant amount of changes from v8.0.0 to v8.1.0 in uuid including some dependencies.

My best guess is that either one of those changes, or change in either of dependencies use some ES6 syntax that uglify doesn't like.

I have tried latest uglify-js@2 - no progress.

I see it this way: we go with highest possible (i.e. 8.0.0) now. I wouldn't expect it ever to become gracefully minified with old uglify-js. Especially that uuid had no releases for months.

Let's build it with 8.0.0

@jrencz jrencz changed the title Use latest version of uuid Use latest (successfully building) version of uuid May 12, 2022
@davecarlson
Copy link

Whoop it passes !

@BourgoisMickael @AllanZhengYP @trivikr can you take another look for code reviewers approval ?

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.

4 participants