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: distribute tsconfig and rollup config files #522

Closed
wants to merge 1 commit into from

Conversation

vegerot
Copy link

@vegerot vegerot commented Aug 19, 2024

Summary:

We currently distribute the source code for the project, but not the
configuration files, meaning that users cannot build the project. This commit
adds the tsconfig.json and rollup.config.js files to the distributed package.

Test Plan:

  • Run npm run build and ensure that the project builds successfully
  • In another project add this package as a dependency and run npm install && cd node_modules/web-vitals && npm install --ignore-scripts && npm run build and ensure that
    the project builds successfully

Motivation:

I am currently working on a patch for the web vitals Chrome extension and need
to use a development build of this web-vitals package. To do that, I want to make a patch like

diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -13,7 +13,7 @@
   "private": true,
   "scripts": {
     "lint": "npx eslint src --fix",
-    "build": "npm install; cp node_modules/web-vitals/dist/web-vitals.attribution.js src/browser_action/web-vitals.js"
+    "build": "npm install && (cd node_modules/web-vitals/ && npm install --ignore-scripts && npm run build) && cp node_modules/web-vitals/dist/web-vitals.attribution.js src/browser_action/web-vitals.js"
   },
   "devDependencies": {
     "babel-eslint": "^10.1.0",
@@ -21,6 +21,10 @@
     "eslint-config-google": "^0.14.0"
   },
   "dependencies": {
-    "web-vitals": "^4.0.0"
+    "web-vitals": "git://github.com/GoogleChrome/web-vitals.git#soft-navs"
+  }

however, the npm run build part fails because we don't distribute
tsconfig.json and rollup.config.js. This commit fixes that.

Summary:

We currently distribute the source code for the project, but not the
configuration files, meaning that users cannot build the project. This commit
adds the tsconfig.json and rollup.config.js files to the distributed package.

Test Plan:

- Run `npm run build` and ensure that the project builds successfully
- In another project add this package as a dependency and run `npm install &&
  cd node_modules/web-vitals && npm install --ignore-scripts && npm run build` and ensure that
  the project builds successfully

Motivation:

I am currently working on a [patch](GoogleChrome/web-vitals-extension#184) for the web vitals Chrome extension and need
to use a development build of this web-vitals package.  To do that, I want to make a patch like

```diff
diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -13,7 +13,7 @@
   "private": true,
   "scripts": {
     "lint": "npx eslint src --fix",
-    "build": "npm install; cp node_modules/web-vitals/dist/web-vitals.attribution.js src/browser_action/web-vitals.js"
+    "build": "npm install && (cd node_modules/web-vitals/ && npm install --ignore-scripts && npm run build) && cp node_modules/web-vitals/dist/web-vitals.attribution.js src/browser_action/web-vitals.js"
   },
   "devDependencies": {
     "babel-eslint": "^10.1.0",
@@ -21,6 +21,10 @@
     "eslint-config-google": "^0.14.0"
   },
   "dependencies": {
-    "web-vitals": "^4.0.0"
+    "web-vitals": "git://github.com/GoogleChrome/web-vitals.git#soft-navs"
+  },
```

however, the `npm run build` part fails because we don't distribute
`tsconfig.json` and `rollup.config.js`.  This commit fixes that.
@vegerot vegerot changed the title chore: distribute tsconfig and rollup config files fix: distribute tsconfig and rollup config files Aug 19, 2024
@tunetheweb
Copy link
Member

I would have thought if you only wanted to build this library, then you'd download the source (i.e. git clone the repo) and then build. So you'd have the tsconfig.json and rollup.config.js files.

Alternatively, if you wanted to to build this as part of a larger app, you'd npm install is as a dependency and use that larger project's typescript and build options.

Is it usually expected to build just the package from npm install? Not sure I've come across that before for a JS library...

@vegerot
Copy link
Author

vegerot commented Aug 19, 2024

Thanks for the quick reply!

Is it usually expected to build just the package from npm install? Not sure I've come across that before for a JS library...

I don't know. My feeling was that if we distribute the src/, the expectation is that users should be able to build it. I understand your feeling that users should just write their own build configuration.

What if the web-vitals package's build steps were more complicated and we still distributed src/? Would it be expected that users reimplement the whole build configuration in order to use the src/ files we already distribute? I get that in this case the build steps are simple so writing a script in the Web Vitals Chrome extension wouldn't be hard.

@vegerot
Copy link
Author

vegerot commented Aug 19, 2024

As a follow-up, what would you suggest I do for GoogleChrome/web-vitals-extension#184 ? Do you think I should git clone this package instead of npm installing it?

@vegerot
Copy link
Author

vegerot commented Aug 19, 2024

idea: can we publish a web-vitals@soft-nav version of this package on npm? That would make the Extension patch the simplest

@tunetheweb
Copy link
Member

It already is published there! And I try to keep it up to date as we make changes to the main branch.

@vegerot
Copy link
Author

vegerot commented Aug 19, 2024

It already is published there! And I try to keep it up to date as we make changes to the main branch.

🤦🏾‍♀️ I swear this was the first place I looked, and after not finding it, I decided to do this more complicated solution. Sorry I can't read!

@vegerot vegerot closed this Aug 19, 2024
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.

2 participants