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

Override default webpack hashing algo #1664

Closed

Conversation

jessp01
Copy link
Contributor

@jessp01 jessp01 commented Aug 30, 2023

This is submitted as a follow-up to this pull: #1654

See https://nodejs.org/en/blog/release/v17.0.0#openssl-30

Since we don't want to require that the node binary be built against OpenSSL 3 (as it narrows down our supported ENVs matrix substantially) we had several options:

  • set NODE_OPTIONS="--openssl-legacy-provider"
  • override default webpack hashing algo

I went with the latter because:
a. MD4 is rubbish anyways
b. NodeJS 16 does not recognise the --openssl-legacy-provider option and so, if set, the build fails with it

Since we don't want to require that the `node` binary be built against
OpenSSL 3 (as it narrows down our supported ENVs matrix substantially)
we had several options:

- set `NODE_OPTIONS="--openssl-legacy-provider"`
- override default webpack hasing algo

I went with the latter because:
a. MD4 is rubbish anyways
b. NodeJS 16 does not recognise the --openssl-legacy-provider option and
so, if set, the build fails.
@jessp01 jessp01 mentioned this pull request Aug 30, 2023
Merged
@jessp01 jessp01 changed the title Override default webpack hasing algo Override default webpack hashing algo Aug 30, 2023
@heff
Copy link
Collaborator

heff commented Sep 6, 2023

Awesome, thanks @jessp01. Is it necessary to check in the dist/ builds with this change? It's causing a merge conflict and the contributing guide says you generally shouldn't.

@jessp01
Copy link
Contributor Author

jessp01 commented Sep 7, 2023

Hi @heff ,

Since looking through the diffs of these minimised files will no doubt prove to be painful and seeing how the build and tests all passed, I reckon it'd be safe to simply override the existing files but I leave the decision to you, of course.

@cookpete
Copy link
Owner

Updating the dist files is part of the npm version and npm publish flow. There’s no need to commit them any other time.

https://github.com/cookpete/react-player/blob/master/CONTRIBUTING.md#dist-files

@jessp01
Copy link
Contributor Author

jessp01 commented Sep 18, 2023

Hi @cookpete ,

Shall we add these to .gitignore to avoid this in future? For now, they can just be reverted to their last version.

@luwes
Copy link
Collaborator

luwes commented Oct 10, 2023

thanks for the contribution!

with this larger refactor it's not an issue anymore. closed by #1684

@luwes luwes closed this Oct 10, 2023
@jessp01
Copy link
Contributor Author

jessp01 commented Oct 10, 2023

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