-
Notifications
You must be signed in to change notification settings - Fork 473
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
feat: scraping expo/react-native version from package.json #838
feat: scraping expo/react-native version from package.json #838
Conversation
8311d51
to
b17c02f
Compare
What's the reason for storing new data in separate files per version, instead of populating library entry in Also, what purpose will serve added |
I have explain about this approach in Evan's notion doc. It's allow us pinned correct version of each library for the RN version.
This is a config file, to say: what are RN and Expo versions we support to looking up. |
I see, this have been added after my first read of the doc. 🙂
Yeah, but I still doesn't see the point of creating separate files, instead of populating entry in {
"name": "expo-av",
...,
"rnVersions": {
"0.64.0" : "0.1.0"
},
"expoVersions": [
{
expo: "45.0.0",
reactNative: "0.68",
packageVersion: "0.2.1",
added: TS,
...
},
{
expo: "44.0.0",
reactNative: "0.64",
packageVersion: "0.1.7",
added: TS,
...
},
...
]
} Both of those fields can also be an array to include object per version, so we can include even more information if needed. Also the older npm releases are always available, so we can fetch few previous versions data per package to get the information for older versions of packages, which removes the need for storing any other JSON files IMO. This also allow us to utilize the new version data in the directory itself without many changes, instead of only serving those data as the
What happens if package uses version outside this list? Does it need to be in file at all, can't we just use the version fields from JSON to establish those lists dynamically? Also there is something important that might not be communicated well, but the committed Saying that, I think that we should still consider migrating to DB first, if possible, which will make managing data and future development on new features way easier. 🙂 |
40825c3
to
2bac2ef
Compare
@Simek : I update the format of file by using the new entry in data.json curl --location --request POST 'http://localhost:3000/api/versions' \
--header 'Content-Type: application/json' \
--data-raw '{"reactNativeVersion":"0.68.0","packages":["expo-av", "@stripe/stripe-react-native", "react-navigation-header-buttons"]}' This request returns: {
"reactNativeVersion": "0.68.0",
"packages": [
{
"npmPackage": "react-navigation-header-buttons",
"versionRange": "9.0.1"
},
{
"npmPackage": "expo-av",
"versionRange": "9.0.0" <---- Incorrect version
},
{
"npmPackage": "@stripe/stripe-react-native",
"versionRange": "0.9.0"
}
]
} But, the request with curl --location --request POST 'http://localhost:3000/api/versions' \
--header 'Content-Type: application/json' \
--data-raw '{"reactNativeVersion":"0.68.0","expoSdkVersion":"45.0.0","packages":["expo-av", "@stripe/stripe-react-native", "react-navigation-header-buttons"]}' {
"reactNativeVersion": "0.68.0",
"expoSdkVersion": "45.0.0",
"packages": [
{
"npmPackage": "expo-av",
"sdkVersion": "45.0.0",
"versionRange": "~11.2.3"
},
{
"npmPackage": "@stripe/stripe-react-native",
"sdkVersion": "45.0.0",
"versionRange": "0.6.0"
},
{
"npmPackage": "react-navigation-header-buttons",
"versionRange": "9.0.1"
}
]
} Because the I still call two endpoints of Expo to avoid copying the data to here then it out-of-sync from the original. When we set up a database/private endpoint for sync data from the expo project. I will remove the call to those endpoints. |
hey @giautm! thanks for putting this together. could you possible update the PR description to include some more information about how this all works? some comments: determining a compatible react-native versionfrom reading the code i can get an idea but i'd like to see a written description of the intent and reasoning behind it. it looks like we chose to use devDependencies, peerDependencies, and dependencies as the basis for a compatible version - this seems reasonable but as you pointed out with the case of expo-av, this won't always be useful. i am somewhat concerned about encouraging developers to depend on expressing react-native version compatibility via peer dependencies, because this means that they need to release a new version of their library for every new react-native version or risk npm 7+ possibly installing an old version of react-native automatically. most libraries don't release new versions every react-native release, and that's fine, but if everyone uses peer dependencies this could become a problem. dev dependencies seems mostly reasonable.. but what if you're working on supporting react-native 0.69 in your module but still support 0.68? as for the plain dependencies field - i don't think any library should ever include react-native as an actual dependency. we could possibly use these values as a best-guess but we may want to introduce a specific field that is only intended for the purpose of telling react native directory what react-native versions are compatible. i'm open to being convinced that this isn't needed though. just concerned about overloading behavior on existing fields as described above. rate limitingdo you think we are going to get rate limited by npm here? each package version lookup seems to be resulting in two more requests to npm. |
Thank @brentvatne for your comment. Determining a compatible react-native versionyes, guess what react-native version is supported based on dependencies should be for reference only as we have no other information. In the future, we may describe a special field to report that. Or, we can also use the information from the
{
"name": "example-a",
"version": "0.2.0",
"@react-native-community/directory": {
"expoSdkVersion": "45.0.0",
"reactNativeVersion": "0.58.0"
}
} The steps will be: check the However, I still think we should have a database, where people can announce the version and their support, and update automatically every time a new version is released. Rate limitingScraping information from NPM should be done only when we first deploy the database, and then updated via webhooks. |
thank you for the clarifications @giautm! this sounds very reasonable.
i agree! would you be able to write up a draft of how this might work? i'm curious to see the vision end to end |
The idea behind the New Version Announcement endpoint is to take in the name of the package, then we're going to scrape the information from NPM, as a trusted source. To avoid users intentionally changing the version of a library not owned by them. And also avoid having to allocate JWT Token for each repository to update their own package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe that we can ship this in its current form, then follow up with further refinements eg: moving to a proper db, being able to announce updates via an endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brentvatne As we speak lately on the Conf, the only blocker form the list for me is the method used for the API calls.
Other things are not major, however still most of them are valid in my eyes and I would like to see them addressed in the future too. @giautm if you have another opinion on some parts, please let me know, I'm sure we can find the way to make most of the feature pod the CLI and Directory side.
hey folks, we are in the process of reworking how things work in Sorry for the delay in replying here btw 🥲 |
@giautm - I think we can ship this if we update to use |
Working on it, sorry about the delay |
2bac2ef
to
647c1cc
Compare
647c1cc
to
ffaf666
Compare
(btw just for cross-reference, this might become overtime - maybe in a second iteration - related to microsoft/rnx-kit#1863 ) |
cc0b2c4
to
a7db733
Compare
@giautm We already have a check for the missing entry in NPM registry: directory/scripts/fetch-npm-data.js Lines 55 to 60 in 71e6d86
Can you explain what issue you are having with Babylon? Existence in NPM registry is perceived as a required, we stopped to throw the build in those case due to templates, which are mostly not published. Babylon addition required changing the GitHub URL regex due to uncommon monorepo structure, maybe we miss something more? |
This is the error related to
|
03752b2
to
0f6e0ea
Compare
Thanks for the details! 👍 Just want to let you know that I have fixed the Babylon issue, but it looks like you have already figure it out too. 🙂 Also the cleanup script requires a change, otherwise it was cleaning |
📝 Why & how
Determining a compatible react-native version
Guess what react-native version is supported based on dependencies should be for reference only as we have no other information. In the future, we may describe a special field to report that. Or, we can also use the information from the
rnx-kit
field to know the supported version.package.json
:The steps will be: check the
@react-native-community/directory
field -> check thernx-kit
field -> check dependencies.However, I still think we should have a database, where people can announce the version and their support, and update automatically every time a new version is released.
✅ Checklist