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

cjs & esm support #224

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

cjs & esm support #224

wants to merge 6 commits into from

Conversation

btakita
Copy link

@btakita btakita commented Dec 6, 2021

The build & tests pass with the added esm support.

Followed the technique in https://redfin.engineering/node-modules-at-war-why-commonjs-and-es-modules-cant-get-along-9617135eeca1

Note this this will require a major version update as the package.json "exports" prop is used. In praxis, there will be no breaking changes unless a dependent imports from an internal module.

From #216 (comment)

Not possible with Elder.js current structure. Open to a PR to help resolve it, but currently don't have a business case for needing to upgrade to esm yet. In fact one of the core packages we rely on doesn't support esm yet so we'll have to wait a bit longer.

Not sure what core package does not support esm. The build & tests pass.

@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2021

Codecov Report

Merging #224 (2cd015b) into master (bf528f1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #224   +/-   ##
=======================================
  Coverage   82.14%   82.14%           
=======================================
  Files          46       46           
  Lines        1960     1960           
  Branches      465      440   -25     
=======================================
  Hits         1610     1610           
  Misses        348      348           
  Partials        2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf528f1...2cd015b. Read the comment docs.

@nickreese
Copy link
Contributor

nickreese commented Dec 7, 2021

Thanks for working on this. 👍

The comment above about it not being possible, I may be wrong on as I may have conflated Elder.js with how we build Elder.js projects.

The core meaningful distinction I remember was that CJS allows the code to be executed before exporting the module. This is something we use aggressively on ElderGuide.com, FindEnergy.com, and other sites to get the most out of caching.

In looking through the Elder.js codebase quickly I couldn't find an example of this happening.

  • Regarding exports: I think that the example repo imports the HookInterface so we'll want to check to make sure that isn't broken on merge.

Package Updates

Thanks for taking a shot at this. I know there are some issues with specific versions of Svelte and it may be worth us pinning some of the other packages as they have been responsible for esoteric issues that are hard to debug.

My gut says we should do a @next release when this is ready and our team will test it on our internal projects to make sure nothing major is broken.

@markjaquith
Copy link
Contributor

Is there any will to resurrect this and work towards getting it merged? I think ESM is the way forward, as increasingly packages are going ESM-only.

I was going to pull in remark-smartypants to do quote-curling in Markdown, but it's ESM only. I had to go resurrect a version of it from 2020, including pinning to an old version of retext. I anticipate this problem becoming worse over time.

Willing to help in any way I can.

@nickreese
Copy link
Contributor

@markjaquith thanks for nudging this. Working on a major refactor of both the plugins and Elder.js core. It will be ESM and have full TS support.

Absolutely huge undertaking as this move required moving all tests to esm as well which jest doesnt support.

You can look at the Typescript branches in both projects… also the published beta on npm.

Probably 3-4 weeks out due to things going on in my personal life and needing to rework the default template.

The changes are almost big enough to warrant a 2.0 as there are a ton of other improvements as well.

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