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

refactor: use import() to enable update of p-map #11147

Closed
wants to merge 3 commits into from
Closed

refactor: use import() to enable update of p-map #11147

wants to merge 3 commits into from

Conversation

voxpelli
Copy link

@voxpelli voxpelli commented Aug 7, 2021

Changes:

Makes use of await import() unblock the update of ESM-only modules like eg. p-map and thus enabling it to be removed from #2958 (comment) and no longer be blocked by #9890.

Context:

As more and more modules are becoming ESM-only, and since Jest is still blocking #9890 from being solved, the challenge of not being able to use ESM-only modules is getting increasingly larger and often the possibility of using await import() is easy to miss.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@voxpelli voxpelli changed the title Use import() to enable update of p-map refactor: use import() to enable update of p-map Aug 7, 2021
@rarkins
Copy link
Collaborator

rarkins commented Aug 7, 2021

This is something I had no idea about until now. Wondering if there's any case studies or technical posts about successfully using this approach?

Downstream I am also using CJS: will this result be compatible?

Will defer to @viceice and @JamieMagee on if this is safe for us to merge.

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this won't solve the issue. This PR will cause runtime issues, as cjs code can't import esm code!

@voxpelli
Copy link
Author

voxpelli commented Aug 7, 2021

This PR will cause runtime issues, as cjs code can't import esm code!

The approach I took here is the second listed solution by SindreSorhus, the creator of eg. p-map and p-queue, on how to use ESM with ones modules. Also eg. wooorm, creator of some of the markdown related dependencies in this project, refers to that guide by SindreSorhus (Sindre is pretty much the one who pushes the hardest in the community for a full move to ESM).

The use of await import() is also noted in the Node.js documentation:

Dynamic import() is supported in both CommonJS and ES modules. In CommonJS modules it can be used to load ES modules.

It is also what I got suggested by jasnell on the Node.js team on Twitter:

To be fair tho, it is possible to use the import() function from inside ESM, so there is a possible path forward there. It's cumbersome, yes, but it works.
[...]
Yep, it works in 12. The import is cached so calling it twice shouldn't have a perf impact.

So, all in all, the use of await import() is 100% compatible with CommonJS in Node.js 🙂 And should be performant as well.

@viceice
Copy link
Member

viceice commented Aug 7, 2021

typescript compiler will convert that to await Promise.resolve(require('p-map')).

So your solution will maybe work for plain JavaScript but not typescript.

@voxpelli
Copy link
Author

voxpelli commented Aug 7, 2021

So your solution will maybe work for plain JavaScript but not typescript.

Didn't know that TypeScript breaks this behavior, here's a tracking bug in TypeScript: microsoft/TypeScript#43329

Have the dynamic import() happen in an uncompiled dependency instead, that way TypeScript can't convert the import() to a require() and thus it will work with ESM modules in TS as well.

Relevant TypeScript issue: microsoft/TypeScript#43329
@viceice
Copy link
Member

viceice commented Aug 7, 2021

Instead of adding those dynamic imports, i would suggest to make renovate fully esm compatible and provide cjs and esm in one package for a while.

So @rarkins can make the hosted app esm compatible smoothly.

Finally we drop cjs build and then update blocked deps as normal.

@viceice viceice closed this Aug 7, 2021
@travi
Copy link
Contributor

travi commented Aug 9, 2021

i would suggest to make renovate fully esm compatible and provide cjs and esm in one package for a while.

be sure to be careful with this approach when esm-only dependencies are involved. if there are dependencies that are esm-only, consumers won't be able to use those dependencies in common-js, even if renovate exports both esm and cjs of itself. the only way that those dependencies could be made compatible, if consumed by renovate as esm-only, would be to bundle the transpiled result into the published renovate exports.

@viceice
Copy link
Member

viceice commented Aug 9, 2021

@travi we don't upgrade to esm deps until renovate itself only publish esm. So no issue with esm imports.

Also renovate isn't ment to be used as library. We always told people that we dont have a stable api. So this nom package should only be used as a cli tool.

Please follow the issue linked for further discussion about renovate esm transition.

@voxpelli
Copy link
Author

voxpelli commented Aug 9, 2021

if there are dependencies that are esm-only, consumers won't be able to use those dependencies in common-js, even if renovate exports both esm and cjs of itself.

@travi This isn’t true, as I outlined in #11147 (comment)

CJS can’t use require() to import ESM, but it absolutely can use await import()

@HonkingGoose
Copy link
Collaborator

Also Renovate isn't meant to be used as library. We always told people that we don't have a stable API. So this npm package should only be used as a CLI tool.

I have seen a lot of the docs, and I don't remember us saying this or something similar...
@viceice Do you maybe know where we mention our API stability, because I cannot find it in our docs. 😉

I did a search for the string API on all our *.md files in VSCode on the current main branch, and came up empty.
I checked our our development docs, design decisions, but that doesn't mention a stable/unstable API as well.

We do have a section in our FAQ, which Renovate versions are officially supported that says:

The Renovate maintainers only support the latest version of Renovate. The Renovate team will only create bugfixes for an older version if the hosted app needs to stay on an older major version for a short time or if some critical bug needs to be fixed and the new major is blocked.

If you're using the hosted app, you don't need to do anything, as the Renovate maintainers update the hosted app regularly. If you're self hosting Renovate, use the latest release if possible.

@viceice
Copy link
Member

viceice commented Aug 9, 2021

We didn't document the internal API because it's not meant to be used by other users. 😉

So not documented means unstable for us. I've discussed this alot with rarkins.

Otherwise we would need a major update on nearly every refactoring.

So users should only depend on the cli or the official hosted app.

@travi
Copy link
Contributor

travi commented Aug 9, 2021

we don't upgrade to esm deps until renovate itself only publish esm. So no issue with esm imports.

good call. i was reading the comment about exporting both cjs and esm for a while as suggesting otherwise, so it seemed worth suggesting caution. sounds like you have your plan sorted out, so feel free to not pay much attention to the suggestion :)

Also renovate isn't ment to be used as library.

yep, i'm aware from an external consumer perspective, but have also seen that it is consumed internally, so i figured mentioning was worthwhile to save internal pain too

This isn’t true, as I outlined in #11147 (comment)

CJS can’t use require() to import ESM, but it absolutely can use await import()

you're right that it is possible. however, at least in my experience, when exporting both cjs and esm, the cjs export tends to be the result of transpilation. as mentioned above for transpiling from typescript, it can be difficult to target import() as a result of such a process. that was the context of my comments, so apologies for the inaccuracy.

it seems my comments caused a bit of disruption here, which wasnt my intent. since it didnt move things forward in a positive direction, please feel free to ignore. i'm working through similar complexity on a number of my projects, so i'm interested in your progress in this area and will keep an eye on the other issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants