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

ES Modules #295

Merged
merged 28 commits into from
Jan 20, 2023
Merged

ES Modules #295

merged 28 commits into from
Jan 20, 2023

Conversation

jcmosc
Copy link
Collaborator

@jcmosc jcmosc commented Jan 9, 2023

(This PR builds off #294)

Convert the project to ES Modules and implements dual CommonJS/ES module exports.

jcmosc added 10 commits January 2, 2023 14:14
Blank binary file was not getting serialized as { type: "Buffer", data: [] }
because read(file) was returning a string instead of Buffer.
Recent versions of node upgraded to OpenSSL 3.0, which deprecated some
older crypto hashing algorithms including md4.

Webpack 4 hard codes the use of md4 ins some places, so we have to set this
flag until we can upgrade Webpack.
Browser tests were failing as polyfill.js was loading a node package in a browser context.
This was when the --openssl-legacy-provider option was introduced.
Prior versions of node do not recognize this option.
Browser tests are failing in the CI environment with:
chokidar@3: Error: Cannot find module 'chokidar'

Don't know why but this thread might be relevant:
facebook/create-react-app#10811
@jcmosc jcmosc mentioned this pull request Jan 9, 2023
…lt property

Without this hack, you would have to do this:

const $RefParser = require("@apidevtools/json-schema-ref-parser).default;

Now you can do this:

const $RefParser = require("@apidevtools/json-schema-ref-parser);
@jcmosc
Copy link
Collaborator Author

jcmosc commented Jan 11, 2023

Hey @philsturgeon this now implements dual CommonJS/ES module support and is ready to merge.

The one thing to call out is that the minimum Node version is now 17, not 14 according to the engines property in package.json. Though it only matters for running tests and it should still work in 14.

@philsturgeon
Copy link
Member

I’m ok increasing versions under those terms as we’re aiming for EAM in a major anyway. Now we’ve merged the other PR can we update and see if this is good to go?

Thank you so much for your support.

@coveralls
Copy link

coveralls commented Jan 11, 2023

Pull Request Test Coverage Report for Build 3897768158

  • 23 of 24 (95.83%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.7%) to 90.943%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/index.js 5 6 83.33%
Totals Coverage Status
Change from base Build 3897133363: 1.7%
Covered Lines: 750
Relevant Lines: 787

💛 - Coveralls

@jcmosc
Copy link
Collaborator Author

jcmosc commented Jan 11, 2023

Yep, looks like all checks have passed

@jcmosc
Copy link
Collaborator Author

jcmosc commented Jan 11, 2023

oh there's conflicts...

@jcmosc
Copy link
Collaborator Author

jcmosc commented Jan 12, 2023

ok fixed

@philsturgeon
Copy link
Member

Thank you @jamesmoschou! This is a great update.

I had to manually release it because semantic-release has gone off the rails, but hopefully thats working ok.

Would you be up for maintaining this package because I am working in the woods 90% of the time now and don't want to be stressing about this when I get back to a computer.

I'm also asking if readmeio#64 can take over, which may just involve merging some of the recent changes and me pointing this package to them.

@jcmosc
Copy link
Collaborator Author

jcmosc commented Jan 21, 2023

Hey @philsturgeon I can help maintain this package. Though I mean maintain in the literal sense, I wouldn't be developing any new feature requests.

Looks like the node engine breaking change snuck in to v9.* version instead of v10. I can update this is you set up the permission :)

@coffeebe4code
Copy link

Sorry, referencing my comment here.
#300 (comment)

It looks like this is the one that broke our project.

@philsturgeon
Copy link
Member

@coffeebe4code fixed in 9.1.2.

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