Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2171503002:
DevTools: Added package.json, LICENSE, and .npmignore (Closed)
|
Created:
4 years, 5 months ago by chenwilliam Modified:
4 years, 5 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Added package.json, LICENSE, and .npmignore
These files are from: https://github.com/paulirish/npm-publish-devtools-frontend
- Will add npm scripts for hosted mode workflow
BUG=629914
Committed: https://crrev.com/530604d9635a00a31329bb2754ce10a5d150e797
Cr-Commit-Position: refs/heads/master@{#406729}
Patch Set 1 #Patch Set 2 : Update with Paul's package.json revision #
Total comments: 4
Patch Set 3 : Address Paul CL feedback #
Total comments: 6
Patch Set 4 : Remove main property from package.json #
Messages
Total messages: 24 (12 generated)
Description was changed from ========== DevTools: add package.json, LICENSE, and .npmignore BUG= ========== to ========== DevTools: Added package.json, LICENSE, and .npmignore These files are from: https://github.com/paulirish/npm-publish-devtools-frontend - I've removed the version property from the package.json since we will be adding the npm package.json version as a last step before publishing to npm. The package.json version update isn't stored as a separate git commit. - Will add npm scripts for hosted mode workflow BUG=629914 ==========
chenwilliam@chromium.org changed reviewers: + lushnikov@chromium.org
Description was changed from ========== DevTools: Added package.json, LICENSE, and .npmignore These files are from: https://github.com/paulirish/npm-publish-devtools-frontend - I've removed the version property from the package.json since we will be adding the npm package.json version as a last step before publishing to npm. The package.json version update isn't stored as a separate git commit. - Will add npm scripts for hosted mode workflow BUG=629914 ========== to ========== DevTools: Added package.json, LICENSE, and .npmignore These files are from: https://github.com/paulirish/npm-publish-devtools-frontend - Will add npm scripts for hosted mode workflow BUG=629914 ==========
paulirish@chromium.org changed reviewers: + paulirish@chromium.org
lgtm % nits https://codereview.chromium.org/2171503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/.npmignore (right): https://codereview.chromium.org/2171503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/.npmignore:6: *.dontupload you can nuke *.dontupload. No idea why it was there. https://codereview.chromium.org/2171503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/LICENSE (right): https://codereview.chromium.org/2171503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/LICENSE:5: // I don't believe we need this authors link in here. Does another license file do this?
https://codereview.chromium.org/2171503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/.npmignore (right): https://codereview.chromium.org/2171503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/.npmignore:6: *.dontupload On 2016/07/20 19:17:52, paulirish wrote: > you can nuke *.dontupload. No idea why it was there. Done. https://codereview.chromium.org/2171503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/LICENSE (right): https://codereview.chromium.org/2171503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/LICENSE:5: // On 2016/07/20 19:17:52, paulirish wrote: > I don't believe we need this authors link in here. Does another license file do > this? This was copied from your license file but I can take it out: https://github.com/paulirish/npm-publish-devtools-frontend/blob/master/src/LI...
The CQ bit was checked by chenwilliam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from paulirish@chromium.org Link to the patchset: https://codereview.chromium.org/2171503002/#ps40001 (title: "Address Paul CL feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm https://codereview.chromium.org/2171503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/package.json (right): https://codereview.chromium.org/2171503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/package.json:3: "version": "1.0.XXXXXX", this doesn't follow semver format, does it? https://codereview.chromium.org/2171503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/package.json:5: "main": "front_end/inspector.html", nit: afaik, the main file should be a *.js so that it would be possible to require this from node
https://codereview.chromium.org/2171503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/package.json (right): https://codereview.chromium.org/2171503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/package.json:3: "version": "1.0.XXXXXX", On 2016/07/20 21:14:24, lushnikov wrote: > this doesn't follow semver format, does it? This isn't actually semver - Paul pinged me about a recent commit he pushed to change the version property: https://github.com/paulirish/npm-publish-devtools-frontend/commit/493ff4cd1d8... I believe we're putting a version placeholder here since we are not actually committing the specific version numbers into the repo and are adding the version right before the npm publish step. https://codereview.chromium.org/2171503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/package.json:5: "main": "front_end/inspector.html", On 2016/07/20 21:14:24, lushnikov wrote: > nit: afaik, the main file should be a *.js so that it would be possible to > require this from node That's a good point. I can update this in a future CL. Should Runtime.js be the entry point? (devtools-frontend/front_end/Runtime.js)
The CQ bit was checked by chenwilliam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2171503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/package.json (right): https://codereview.chromium.org/2171503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/package.json:5: "main": "front_end/inspector.html", On 2016/07/20 21:29:53, Will_Chen wrote: > On 2016/07/20 21:14:24, lushnikov wrote: > > nit: afaik, the main file should be a *.js so that it would be possible to > > require this from node > > That's a good point. I can update this in a future CL. Should Runtime.js be the > entry point? (devtools-frontend/front_end/Runtime.js) I'd drop it if possible (Runtime.js doesn't make much sense as well)
https://codereview.chromium.org/2171503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/package.json (right): https://codereview.chromium.org/2171503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/package.json:5: "main": "front_end/inspector.html", On 2016/07/20 21:34:08, lushnikov wrote: > On 2016/07/20 21:29:53, Will_Chen wrote: > > On 2016/07/20 21:14:24, lushnikov wrote: > > > nit: afaik, the main file should be a *.js so that it would be possible to > > > require this from node > > > > That's a good point. I can update this in a future CL. Should Runtime.js be > the > > entry point? (devtools-frontend/front_end/Runtime.js) > > I'd drop it if possible (Runtime.js doesn't make much sense as well) Done.
The CQ bit was checked by chenwilliam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from paulirish@chromium.org, lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2171503002/#ps60001 (title: "Remove main property from package.json")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== DevTools: Added package.json, LICENSE, and .npmignore These files are from: https://github.com/paulirish/npm-publish-devtools-frontend - Will add npm scripts for hosted mode workflow BUG=629914 ========== to ========== DevTools: Added package.json, LICENSE, and .npmignore These files are from: https://github.com/paulirish/npm-publish-devtools-frontend - Will add npm scripts for hosted mode workflow BUG=629914 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: Added package.json, LICENSE, and .npmignore These files are from: https://github.com/paulirish/npm-publish-devtools-frontend - Will add npm scripts for hosted mode workflow BUG=629914 ========== to ========== DevTools: Added package.json, LICENSE, and .npmignore These files are from: https://github.com/paulirish/npm-publish-devtools-frontend - Will add npm scripts for hosted mode workflow BUG=629914 Committed: https://crrev.com/530604d9635a00a31329bb2754ce10a5d150e797 Cr-Commit-Position: refs/heads/master@{#406729} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/530604d9635a00a31329bb2754ce10a5d150e797 Cr-Commit-Position: refs/heads/master@{#406729} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
