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

Reinstate install from Git #21656

Merged
merged 1 commit into from
Dec 28, 2023
Merged

Conversation

ddbeck
Copy link
Collaborator

@ddbeck ddbeck commented Dec 18, 2023

Summary

Reinstate the option to install BCD from Git. This is useful to, for example, run analysis against the data of a BCD PR, before it's merged and released.

Test results and supporting details

  1. Run npm install git+https://github.com/mdn/browser-compat-data.git and attempt to import or require BCD. 😢
  2. Run npm install git+https://github.com/ddbeck/browser-compat-data.git#installable-from-HEAD and attempt to import or require BCD. 😄

Related issues

Regress originated at #16593.

@github-actions github-actions bot added infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project dependencies ⛓️ Pull requests that update a dependency package or file. labels Dec 18, 2023
@ddbeck ddbeck force-pushed the installable-from-HEAD branch 2 times, most recently from 8499777 to f68a985 Compare December 18, 2023 12:32
@ddbeck ddbeck marked this pull request as draft December 18, 2023 12:34
@ddbeck ddbeck marked this pull request as ready for review December 18, 2023 12:35
queengooborg

This comment was marked as outdated.

@ddbeck
Copy link
Collaborator Author

ddbeck commented Dec 18, 2023

@queengooborg I'm very confused by your comment. What build artifacts are being tracked by Git here? I haven't touched .gitignore nor have I checked in data.json. It's not clear to me how my changes would require such. Could you spell this out for me?

@queengooborg
Copy link
Collaborator

Oh...I'm sorry, I misread what file is being touched. I thought you were updating .gitignore, not .npmignore.

@queengooborg queengooborg dismissed their stale review December 18, 2023 17:49

Misread the changes this PR is performing

Copy link
Collaborator

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

Okay, now that I've properly read what exactly this PR is doing, this is LGTM. Apologies for the previous review, I panicked because I thought that .gitignore was being updated, not .npmignore.

Small nit: we could actually probably just remove .npmignore altogether? Since we deploy from the build/ folder, the .npmignore doesn't really have any effect.

@queengooborg queengooborg merged commit 0209395 into mdn:main Dec 28, 2023
6 checks passed
@ddbeck ddbeck deleted the installable-from-HEAD branch January 2, 2024 13:10
@ddbeck
Copy link
Collaborator Author

ddbeck commented Jan 2, 2024

Thanks, @queengooborg!

Small nit: we could actually probably just remove .npmignore altogether? Since we deploy from the build/ folder, the .npmignore doesn't really have any effect.

I'll put it on my to do list to experiment with this a bit. IIRC, in the absence of a .npmignore, npm uses .gitignore. We might still need an empty .npmignore, for the changes in this PR to continue to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies ⛓️ Pull requests that update a dependency package or file. infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants