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

continuing esmification #288

Closed
wants to merge 8 commits into from
Closed

continuing esmification #288

wants to merge 8 commits into from

Conversation

philsturgeon
Copy link
Member

@philsturgeon philsturgeon commented Nov 7, 2022

Based on #268

  • chore: switched tests from require to import

@philsturgeon
Copy link
Member Author

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
Copy link
Member Author

philsturgeon commented Nov 7, 2022

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
Copy link
Contributor

wparad commented Nov 7, 2022

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 philsturgeon mentioned this pull request Nov 7, 2022
still getting issues, looks like export default {

  /**
   * Returns the current working directory (in Node) or the current page URL (in browsers).
   *
   * @returns {string}
   */
  cwd () {
does not actually make import { cwd } available?
@philsturgeon
Copy link
Member Author

@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
Copy link
Contributor

wparad commented Nov 7, 2022

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
Copy link
Member Author

philsturgeon commented Nov 25, 2022

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
Copy link
Member Author

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

@wparad
Copy link
Contributor

wparad commented Nov 25, 2022

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 )

This was referenced Nov 26, 2022
@wparad
Copy link
Contributor

wparad commented Nov 29, 2022

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
Copy link

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: #290 (comment)

@aerialist7
Copy link

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
Copy link
Collaborator

jcmosc commented Jan 9, 2023

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:
#294

This PR converts this project to ES modules:
#295

What I haven't done is provide dual CommonJS/ES module support as this thread has discussed. It would be possible similar to #290 (comment) - just have to bundle up the ESM files and reference from package.json.

@jcmosc
Copy link
Collaborator

jcmosc commented Jan 11, 2023

#295 now implements dual CommonJS/ES module support.

@philsturgeon philsturgeon deleted the esmification branch January 20, 2023 19:12
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.

5 participants