APIDevTools / json-schema-ref-parser

Parse, Resolve, and Dereference JSON Schema $ref pointers in Node and browsers
https://apitools.dev/json-schema-ref-parser
MIT License
952 stars 227 forks source link

continuing esmification #288

Closed philsturgeon closed 1 year ago

philsturgeon commented 1 year ago

Based on #268

philsturgeon commented 1 year ago

Fixed the conflicts from the original PR and switched the tests over to import, but getting this error:


Error [ERR_UNSUPPORTED_DIR_IMPORT]: Directory import '/Users/phil/src/json-schema-ref-parser/' is not supported resolving ES modules imported from /Users/phil/src/json-schema-ref-parser/test/specs/absolute-root/absolute-root.spec.js
    at new NodeError (node:internal/errors:372:5)
    at finalizeResolution (node:internal/modules/esm/resolve:433:17)
    at moduleResolve (node:internal/modules/esm/resolve:1009:10)
    at defaultResolve (node:internal/modules/esm/resolve:1218:11)
    at ESMLoader.resolve (node:internal/modules/esm/loader:580:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:294:18)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:80:40)
philsturgeon commented 1 year ago

Ah dammit ok, this is stuck on directory imports being used everywhere and it's not just a case of shoving a .js which is easy enough.

import { abs, url as _url } from "../../utils/path";
import { abs, url as _url } from "../../utils/path.js";

That works, but wtf do I do with import $RefParser from "../../..";?

wparad commented 1 year ago

Ah dammit ok, this is stuck on directory imports being used everywhere and it's not just a case of shoving a .js which is easy enough.

import { abs, url as _url } from "../../utils/path";
import { abs, url as _url } from "../../utils/path.js";

That works, but wtf do I do with import $RefParser from "../../..";?

Is there a reason that wouldn't be import $RefParser from '../../../lib'?

Also you shouldn't need to put .js on the end of those files, you can change the config to automatically pull in .js files when doing the resolution.

philsturgeon commented 1 year ago

@wparad I don't know what I'm doing, which is a lot of the issue.

I have run around adding lots of .js to everything which was labourious and probably incomplete, so if there's a way to automatically pull in .js that'll be lovely. Let's do that.

Can you show me how?

A bigger issue now is:

file:///Users/phil/src/json-schema-ref-parser/test/specs/absolute-root/absolute-root.spec.js:6
import { cwd } from "../../../lib/util/url.js";
         ^^^
SyntaxError: The requested module '../../../lib/util/url.js' does not provide an export named 'cwd'

The approach being used in that library is this:


export default {

  cwd () {

and im not sure thats the right way to name exports to be used like this?

import { cwd } from "../../../lib/util/url.js";
wparad commented 1 year ago

For the second one it is export function cwd() { /* implementation */ }

For the first one, I assume it's a babel or eslint issue more than it is anything else, but I would need to see the actual error to suggest a fiix.

philsturgeon commented 1 year ago

Thanks @wparad. I've brought in the index.mjs change from @kevinswiber and removed all the .js suffixes, but that's giving me a bunch of errors:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/phil/src/json-schema-ref-parser/lib/index' imported from /Users/phil/src/json-schema-ref-parser/test/specs/absolute-root/absolute-root.spec.js
    at new NodeError (node:internal/errors:372:5)
    at finalizeResolution (node:internal/modules/esm/resolve:437:11)
    at moduleResolve (node:internal/modules/esm/resolve:1009:10)
    at defaultResolve (node:internal/modules/esm/resolve:1218:11)
    at ESMLoader.resolve (node:internal/modules/esm/loader:580:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:294:18)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:80:40)

Looks like import needs a file extension.

Mandatory file extensions

A file extension must be provided when using the import keyword to resolve relative or absolute specifiers. Directory indexes (e.g. './startup/index.js') must also be fully specified.

This behavior matches how import behaves in browser environments, assuming a typically configured server.

https://nodejs.org/api/esm.html#esm_import_statements

I am fumbling around in the dark trying to finish up other peoples pull requests here and I really don't have time for it. Can somebody please help me get this pull request working, by running npm run build && npm test? I would really appreciate it, as would the thousands of people using this module.

philsturgeon commented 1 year ago

Calling on @aerialist7 to help get this done, or anyone.

wparad commented 1 year ago

Okay so it seems resolving without the extension is still experimental. If we want to do it anyway we need something like babel, and don't we need babel anyway if we want to provide support for everyone using the library that isn't using esm?

Or is the plan to force everyone using the library to convert to es modules if they want to use it?

(Just a quick reference on cjs usage) (babel esm to cjs reference )

wparad commented 1 year ago

Oh I forgot I wrapped this library not long ago to specifically add cjs/esm/browser support, there are some minor changes in this repo which might offer some inspiration into supporting the babelify: https://github.com/Rhosys/openapi-resolver.js

kevinswiber commented 1 year ago

I started playing around with this branch. If no one beats me to it, I can try to get something out this week. I don't think we'll need a separate build step. See: https://github.com/APIDevTools/json-schema-ref-parser/issues/290#issuecomment-1331009231

aerialist7 commented 1 year ago

Hi guys, sorry, but I have no time now to continue esmification process. You are welcome to finish it! If you have any questions I'll try to answer

jcmosc commented 1 year ago

Hey all, I've tried to finish this in a different PR since I wanted to make sure that CI tests were passing first, since they haven't been passing for a while now:

This PR fixes tests to provide a baseline: https://github.com/APIDevTools/json-schema-ref-parser/pull/294

This PR converts this project to ES modules: https://github.com/APIDevTools/json-schema-ref-parser/pull/295

What I haven't done is provide dual CommonJS/ES module support as this thread has discussed. It would be possible similar to https://github.com/APIDevTools/json-schema-ref-parser/issues/290#issuecomment-1331009231 - just have to bundle up the ESM files and reference from package.json.

jcmosc commented 1 year ago

https://github.com/APIDevTools/json-schema-ref-parser/pull/295 now implements dual CommonJS/ES module support.