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

feat: make the lib isomorphic #280

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

alanpoulain
Copy link
Contributor

Fixes #275.

In order to make this lib isomorphic (usable both in the browser and in Node.js), these changes have been made:

  • do not use process global variable directly
  • use fetch instead of http (Fetch API is available in Node.js as experimental and if not, can be polyfilled easily).
  • do not use url (use URL API instead).

@philsturgeon
Copy link
Member

Thank you so much @alanpoulain!

Linter has noticed a few things:


/home/runner/work/json-schema-ref-parser/json-schema-ref-parser/lib/resolvers/http.js
Error:     3:7   error  'http' is assigned a value but never used   no-unused-vars
Error:     4:7   error  'https' is assigned a value but never used  no-unused-vars
Error:   148:13  error  Strings must use doublequote                quotes
Error:   150:48  error  Strings must use doublequote                quotes
Error:   150:60  error  Strings must use doublequote                quotes

/home/runner/work/json-schema-ref-parser/json-schema-ref-parser/lib/util/url.js
Warning:   25:18  warning  'url' is already declared in the upper scope  no-shadow

I would just fix those myself but that last one might need a closer eye to make sure there's nothing funny going on.

@philsturgeon
Copy link
Member

I had a go at fixing the linting issues myself in #282 but there is a test failure too.

  148 passing (525ms)
  1 failing

  1) HTTP options
       http.headers
         should override default HTTP headers:
     ResolverError: Error reading file "https://httpbin.org/headers"
      at onError (lib/parse.js:32:232)

Can you make sure npm run test:node is working before you push?

Thank you!

@alanpoulain alanpoulain force-pushed the feat/isomorphic branch 2 times, most recently from be675f3 to 4081adc Compare October 11, 2022 14:15
@alanpoulain
Copy link
Contributor Author

alanpoulain commented Oct 11, 2022

Sorry I forgot to run the linter, it should not be an issue anymore!

For the test failure, I was testing with Node.js 18, hence I didn't have the failure.
It should be fixed now: I added the node-fetch polyfill in the tests.
I've also added node-abort-controller for Node.js 14 tests.

I have updated the CI as well.

I've also added a mention to use a polyfill in the README.

@philsturgeon
Copy link
Member

Thank you for such a speedy fix!

There's one more issue popping up on Node v14 and I have seen it in a few places, not sure why...

Run npm run coverage:node

> @apidevtools/[email protected] coverage:node D:\a\json-schema-ref-parser\json-schema-ref-parser
> nyc node_modules/mocha/bin/mocha



  File names with special characters
    1) should parse successfully


  0 passing (47ms)
  1 failing

  1) File names with special characters
       should parse successfully:
     TypeError [ERR_INVALID_URL]: Invalid URL: specs/__(%7B%5B%20%25%20&%20$%20%23%20@%20%60%20~%20,)%7D%5D__/__(%7B%5B%20%25%20&%20$%20%23%20@%20%60%20~%20,)%7D%5D__.yaml
      at new NodeError (internal/errors.js:322:7)
      at onParseError (internal/url.js:270:9)
      at new URL (internal/url.js:346:5)
      at Object.resolve (lib\util\url.js:8:137)
      at $RefParser.parse (lib\index.js:46:39)
      at Context.<anonymous> (test\specs\__({[ % & $ # @ ` ~ ,)}]__\special-characters.spec.js:13:33)
      at processImmediate (internal/timers.js:464:21)

Any chance you can smash that one too?

@alanpoulain
Copy link
Contributor Author

That's weird, I cannot reproduce it on my machine, it seems a Windows only issue.
It doesn't seem related to the changes of this PR too.
I won't have time to look at it before a few days.

@philsturgeon philsturgeon merged commit 138f648 into APIDevTools:main Oct 12, 2022
@philsturgeon
Copy link
Member

@alanpoulain thank you, I'll make a fresh issue for it. Merged!

@lucy-loo
Copy link

@philsturgeon can you publish a new version? 🙏

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.

Improve compatibility with browser by using better APIs
3 participants