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

Esmification #268

Closed
wants to merge 2 commits into from
Closed

Esmification #268

wants to merge 2 commits into from

Conversation

aerialist7
Copy link

Hi Team,
I've prepare the ESM version of the library.
Can you please review the changes and merge them if OK?

@philsturgeon philsturgeon requested a review from P0lip June 15, 2022 14:03
@aerialist7
Copy link
Author

aerialist7 commented Jun 15, 2022

Hi @P0lip, hi @philsturgeon

I've fixed the ESLint, but seems tests will fail because of type: "module".

Can you consult me, how to correctly update project config to enable es-modules for test environment?
P.S. Here you can find a draft of tests esmification - https://github.com/aerialist7/json-schema-ref-parser/tree/esm-tests

@philsturgeon

This comment was marked as outdated.

@philsturgeon
Copy link
Member

@aerialist7 I understand this situation a lot better now, and I see that there's a way to go ahead with this pull request if you're still interested!

https://dev.to/bcoe/esm-doesn-t-need-to-break-the-ecosystem-4p8b

They talk about conditional imports:

  {
    "exports": {
      ".": {
        "import": "./index.mjs",
        "require": "./index.cjs"
      },
      "./helpers": {
        "import": "./helpers/helpers.mjs",
        "require": "./helpers/index.js"
      }
    }
  }

Can we set that up so things will continue to work for CJS and ESM?

Then we'll avoid the problems discussed in the node-fetch issue.

@aerialist7
Copy link
Author

Hi @philsturgeon, thank you for your respond.
I think we can, but we need to configure build to CJS and ESM together.

@philsturgeon
Copy link
Member

I'll admit I'm not caught up with all this, but I have blundered my way through it with help on other projects.

https://github.com/stoplightio/spectral-owasp-ruleset/blob/main/package.json

We got that repo building from TS to CJS and ESM so perhaps something can be nicked from there.

Do you think you could take a swing at it?

@philsturgeon
Copy link
Member

I've just merged two large changes which will have caused some conflicts here, sorry about that. I'm done merging big changes now though so you've got a clear runway to land this change.

@deepakthankachan
Copy link

any updates on this? cheers

@philsturgeon
Copy link
Member

@deepakthankachan you know if this PR has updates because it will show them. Currently there are conflicts and failing tests so we're just waiting for that to get fixed.

@wparad
Copy link
Contributor

wparad commented Oct 27, 2022

The best way to proceed with this is first add in building the library with a tool that can output both esm and cjs and then do the esm changes if we want. There shouldn't really be a reason we need to introduce esm though, since esm can already import cjs files which the library currently is. If we see the value in converting to esm in the long run then making those changes piecemeal in follow up PRs is the best way to go.

@philsturgeon
Copy link
Member

@acunniffe any chance you can pick this PR up and get it over the line?

@philsturgeon
Copy link
Member

philsturgeon commented Nov 7, 2022

I've made progress over here if anyone is interested in continuing this effort. #288

@philsturgeon
Copy link
Member

More progress is being made in #288 and this one isn't being worked on so I'll close this for that.

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.

None yet

4 participants