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

fix #14: make resolve synchronous #15

Merged
merged 10 commits into from Apr 24, 2023
Merged

Conversation

giltayar
Copy link
Contributor

@giltayar giltayar commented Apr 13, 2023

As described in issue #14, import.meta.resolve is becoming synchronous both in the browser and in Node.js:

You can see it here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/import.meta/resolve

And also in Node.js, where a PR just changed it to be synchronous: nodejs/node#44710

This PR makes resolve synchronous to fit the above.

Note that the baseline test calling import.meta.resolve was not changed to be synchronous,
because that change will probably be merged into Node v19,
and so for 18 import.meta.resolve is still synchronous.

@giltayar giltayar changed the title fix #14: make resolve synchronous fix #14: make resolve synchronous Apr 13, 2023
Copy link
Owner

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Thanks Gil for working on this! Much appreciated!

Some notes!

index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/core.js Outdated Show resolved Hide resolved
script.js Outdated Show resolved Hide resolved
giltayar and others added 3 commits April 13, 2023 20:42
Better return value description

Co-authored-by: Titus <tituswormer@gmail.com>
Signed-off-by: Gil Tayar <gil@tayar.org>
Co-authored-by: Titus <tituswormer@gmail.com>
Signed-off-by: Gil Tayar <gil@tayar.org>
@wooorm
Copy link
Owner

wooorm commented Apr 14, 2023

Thinking more about https://github.com/wooorm/import-meta-resolve/pull/15/files#r1165850902, how strongly do you feel about releasing this now?

Given that: a) this isn’t supported anywhere yet, b) this is a major, c) Node 16 is dropped at the end of the month, we could also wait until April 30, and drop Node 16 too?

@giltayar
Copy link
Contributor Author

giltayar commented Apr 15, 2023

I think you're right and waiting until end of April and dropping support of 16 is a good idea. And maybe release this only when the change really is in Node and we can check the baseline against it. BTW, my guess is that this change to import.meta.resolve won't be backported to Node 18 anyway, so we'll have to live with dual sync and async import.meta.resolve in Node.js anyway.

Note that while this is a breaking change, it won't really break any code, given that await import.meta.resolve(...) works even when the function doesn't return a promise. It might break some linter/TS rules, but that's about it.

TL;DR. I suggest we wait for two things:

  1. Drop of Node 16 at end of April
  2. Checking this against the version of Node that implemented this change, so we can implement two baselines test: one for version 18 (async) and one for version 20 (sync). I'm closely following this, so I can both know when it happens and implement the change.

@giltayar
Copy link
Contributor Author

Node 20 is out and import.meta.resolve is now synchronous on it, and can be checked! Since this PR is still open, I'll continue it and create two baselines: an async one for "Node <20' and a sync one for 'Node>20` as we discussed above.

@giltayar
Copy link
Contributor Author

I implemented (and manually tested on 16, 18, 20) the synchronous baseline test, and added an async one that runs for versions < 20.

I changed the action main.yml to test on 18 and latest, but I wasn't sure about that, so it's a separate commit.

Copy link
Owner

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Really appreciate your continued work Gil! Some tiny nits and then it’s ready I think!

.github/workflows/main.yml Outdated Show resolved Hide resolved
test/core.js Outdated Show resolved Hide resolved
test/core.js Show resolved Hide resolved
@giltayar
Copy link
Contributor Author

Thanks! Are you now going to create a release (I think you should 🙏). I know that @nicolo-ribaudo is waiting to use this version instead of inlining the code (see the issue linked above). I also did the same in the https://github.com/eslint-community/eslint-plugin-n code (this is what brought me here to help here 😁), so I can also now kill the inlining hack there too.

@wooorm wooorm merged commit dcaeda3 into wooorm:main Apr 24, 2023
4 checks passed
@wooorm
Copy link
Owner

wooorm commented Apr 24, 2023

Yes!
I don’t think I need to wait for April 30, for Node 16 EOL, I can also just do that now.

Anything else you want to get in the release?

@wooorm
Copy link
Owner

wooorm commented Apr 24, 2023

Released in https://github.com/wooorm/import-meta-resolve/releases/tag/3.0.0!

@nicolo-ribaudo
Copy link

nicolo-ribaudo commented Apr 24, 2023

Thank you both!

@jakebailey
Copy link
Contributor

Is there a major need to drop <18 if await resolve is a no-op if the result isn't a promise? As far as I can tell, v3.0.0 still works back to Node 14. EOL or not, I have a vested interest in keeping Node 14 going (as that's what TypeScript is going to be supporting for the forseeable future, and this package is a dep of the build system I wrote).

I can just not upgrade, of course; 2.2.2 works fine, but dropping Node 14/16 before they're EOL seems quite strong! 😄

@wooorm
Copy link
Owner

wooorm commented Apr 24, 2023

I made a mistake with 16 being EOL, that is supposed to be in a year, but is instead in 6 months.

However, Node 14 is EOL this week. That is intentional

@wooorm
Copy link
Owner

wooorm commented Apr 25, 2023

@jakebailey Given that 14 is EOL in 5 days, why do you think this project should support that for the foreseeable future?

(reference: https://github.com/nodejs/Release#release-schedule)

@jakebailey
Copy link
Contributor

Well, if the current code supports it, I really only meant that Node 14 could be run in CI and probably continue to work until it doesn't. And maybe if it does break, the fix will be small, or if it isn't, then 14 can be dropped.

But, it's your library! I'm content just using 2.2.2 which works back to Node 12.

Thankfully you aren't using engines so older Node environments don't warn/error out no matter what anyhow.

@wooorm
Copy link
Owner

wooorm commented Apr 25, 2023

The problem for me is that there aren’t really good reasons to switch the code style to depend on a new Node version, for example, Object.hasOwn could be a reason, but well that isn’t super useful. That’s not worth a major.

But there are good reasons to switch the infra to depend on a new Node version. E.g., node:test or so.
So it would be nice for the tooling in this project to work in maintained versions, but not be limited to unmaintained versions.

Hence why I am currently thinking that the best way to go about it is, if a major is going to happen anyway, to drop support for unmaintained Node versions. And describe that as such. Even if the code might currently work, it allows for future patch/minor releases with new/changed dependencies or more modern syntax that only work in those versions too.

Open to ideas!

I’ve updated the readme/actions to check Node 16 again, and amended the release notes to specify Node 16 too.

@jakebailey
Copy link
Contributor

It's totally fine to drop 14 if you don't want to deal with it. I'm just being a little more conservative for TS given there are always people who can't migrate up. But, realisticaly, those people are probably not running TS 5.x either.

Based on the linked thread, seems like babel is dropping it too, though whether or not that's because of the change in this library, I'm unsure.

@giltayar giltayar deleted the sync-resolve branch April 26, 2023 08:02
@nicolo-ribaudo
Copy link

Babel already has a build step around this library, so the choice here doesn't really affect our choice.

It's possible that we'll keep Node.js 14 compatibility, in which case we'll just transpile usages of JS functions introduced in Node.js 16 to something that works in Node.js 14.

christophehurpeau added a commit to christophehurpeau/check-package-dependencies that referenced this pull request May 8, 2023
christophehurpeau added a commit to christophehurpeau/check-package-dependencies that referenced this pull request May 8, 2023
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