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

Bump Npm.depends packages for fetch #12739

Merged
merged 17 commits into from Oct 3, 2023

Conversation

jamauro
Copy link
Contributor

@jamauro jamauro commented Aug 9, 2023

Bump node-fetch and whatwg-fetch to the latest versions

Bump node-fetch and whatwg-fetch to the latest versions
@jamauro jamauro changed the title Bump Npm.depends packages Bump Npm.depends packages for fetch Aug 9, 2023
@Grubba27 Grubba27 added this to the Release 2.14 milestone Aug 10, 2023
Copy link
Contributor

@Grubba27 Grubba27 left a comment

Choose a reason for hiding this comment

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

For some reason tests are failing can you check on that?

@jamauro
Copy link
Contributor Author

jamauro commented Aug 10, 2023

It's failing here

return fetch(Meteor.absoluteUrl("/__meteor__/dynamic-import/fetch"), {

with

TypeError: fetch is not a function

I thought fetch was global so I'm not sure what the underlying issue is. I was hoping someone closer to the code might be able to figure it out. I suppose fetch could be explicitly imported into that test file with import { fetch } from 'meteor/fetch'.

I also noticed that the dynamic-import tests are using fetch@0.1.1 here:
https://github.com/meteor/meteor/blob/8818ccef3736f878c9744b701927768ae9b22643/tools/tests/apps/dynamic-import/.meteor/packages#L27C4-L27C4

Shouldn't this just be fetch? I'm not sure if that is intentional or an oversight.

@Grubba27
Copy link
Contributor

I also noticed that the dynamic-import tests are using fetch@0.1.1 here:
https://github.com/meteor/meteor/blob/8818ccef3736f878c9744b701927768ae9b22643/tools/tests/apps/dynamic-import/.meteor/packages#L27C4-L27C4

Maybe needs to be updated ? I saw theses days a test that only passed after we updated the .meteor/package file

From https://github.com/node-fetch/node-fetch#loading-and-configuring-the-module:

> node-fetch from v3 is an ESM-only module - you are not able to import it with require()
@Grubba27
Copy link
Contributor

Tinytest.add("fetch - sanity", function (test) {
test.equal(typeof fetch, "function");
});

this test failed as well in the CI

@jamauro
Copy link
Contributor Author

jamauro commented Aug 11, 2023

@Grubba27 @denihs I think I'm stuck on this PR. It looks like the fetch package is required to use ecmascript 5 syntax per Meteor's check-legacy-syntax. node-fetch has these requirements: https://github.com/node-fetch/node-fetch#loading-and-configuring-the-module

So I think the options are:

  1. Remove fetch from the check-legacy-syntax.js here:


    so that we can use import fetch from 'node-fetch'

  2. Pin fetch to node-fetch@2 and continue using const fetch = require('node-fetch'). IMO, this isn't great.

  3. Another idea that y'all have :)

@jamauro
Copy link
Contributor Author

jamauro commented Aug 11, 2023

  1. Pin fetch to node-fetch@2 and continue using const fetch = require('node-fetch'). IMO, this isn't great.

I went with this option for now. If y'all have a better solution, please let me know. I don't love being pinned to version 2 of node-fetch but they do say it will continue to receive critical bug fixes and it does fix the issue I was running into.

Still it seems like Meteor should be able to use the latest and greatest. I'm sure there was a good reason to force some core packages to use ecmascript 5 – I'm guessing backwards compatibility – but maybe that restriction can/should be lifted in Meteor 3.0+? Curious to hear what y'all think.

@harryadel
Copy link
Contributor

harryadel commented Aug 18, 2023

Does this mean this is closed?
#11927

@harryadel
Copy link
Contributor

Do we have a release date on 2.14? cc @Grubba27 @denihs I understand 3.0 is taking most of your time but #12785 with other stuff need to be released soon and no work on 2.14 has been done so far, even the creation of a PR!

Again, thank you for your work. This is just a friendly reminder.

@StorytellerCZ
Copy link
Collaborator

@harryadel I will be pushing @Grubba27 to make 2.14 the Hacktoberfest release! I think it could become a nice tradition going forward.

@Grubba27 Grubba27 changed the base branch from devel to release-2.14 October 3, 2023 12:48
@Grubba27 Grubba27 merged commit 8b25696 into meteor:release-2.14 Oct 3, 2023
6 checks passed
@jamauro
Copy link
Contributor Author

jamauro commented Oct 3, 2023

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants