-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Resolve transitive dependencies based on package.json #3286
base: v2
Are you sure you want to change the base?
Conversation
Resolve transitive dependencies' versions based on package.json
@microsoft-github-policy-service agree company="wix" |
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.
See small comments regarding naming.
I suggest adding a bit more context in the PR description to explain the motivation. I think an example will help the typescript team understand what you did here
packages/ata/src/index.ts
Outdated
let typesPackageJson | ||
if (config.usePackageJson){ | ||
typesPackageJson = moduleMap.get(dts.moduleName)?.typesPackageJson | ||
} |
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.
let typesPackageJson | |
if (config.usePackageJson){ | |
typesPackageJson = moduleMap.get(dts.moduleName)?.typesPackageJson | |
} | |
const typesPackageJson = config.usePackageJson && moduleMap.get(dts.moduleName)?.typesPackageJson |
To be clear, is the intent here to ensure that all dependencies are loaded transitively? Should this flag be set for the website? Should it be disabled at all? |
@jakebailey The intent is to allow resolution of transitive (indirect) dependencies according to their module's package.json file. Before this change, any indirect dependency gets resolved to latest, since the only way to control versioning is via the For example, if the input file says
|
@jakebailey Hi, is any additional explanation needed before a review? |
@jakebailey Hi again, any updates here? Let me know if any additional explanation is needed :) |
The main thing I would like to see is this flag be enabled for ATA on the playground, then be tested there (which I can deploy from this PR). |
@jakebailey Hi Jake, I enabled the flag on the playground. For better clarification, this is the new functionality:
As for tests - I didn't find any ATA related tests under the |
There aren't any tests, really, not that I'm aware of. |
TBH I feel like the website would be better served by https://www.npmjs.com/package/@vscode/ts-package-manager / https://github.com/microsoft/vscode/blob/main/extensions/typescript-language-features/src/filesystems/autoInstallerFs.ts which actually uses a package manager to do resolution and place files into the VFS, but that's a larger undertaking. I'd just worry that this PR is potentially wrong for things like version constraints (not that the current code is particularly great on that front either). If you weren't trying to plug this into the playground originally, why were you updating the ATA package? Where are you using it? |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://victorious-plant-05c166c10-3286.centralus.5.azurestaticapps.net |
We aim to use ATA in our monaco-based code editor. I'm aware of the fact that the solution is far from perfect but it's more accurate then the existing resolution to latest versions of transitive dependencies |
When the newly added
resolveDependenciesFromPackageJson
flag is set to true, ATA will resolve the versions of any transitive dependency according to thepackage.json
file of the same module (unless any// types:
comment is specified).