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

Only use our npm for @salesforce packages #611

Merged
merged 9 commits into from
Aug 31, 2018

Conversation

vazexqi
Copy link
Contributor

@vazexqi vazexqi commented Aug 31, 2018

What does this PR do?

This makes it fetch from the public npm for the public packages, possibly reducing any mirroring issues.

What issues does this PR fix or reference?

Build issues

"eslint": "4.16.0",
"eslint-plugin-lwc": "0.4.0",
"lwc-language-server": "1.6.6",
"eslint-plugin-lwc":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lcampos - npm also has a feature to specify a tarball URL. See https://docs.npmjs.com/files/package.json#urls-as-dependencies

So I used that instead of bundling it directly. I got this url using, e.g., npm view --registry=https://npm.lwcjs.org/ [email protected]

@vazexqi vazexqi requested review from lcampos and rsalvador August 31, 2018 17:24
"eslint-plugin-lwc": "0.4.0",
"lwc-language-server": "1.6.6",
"eslint-plugin-lwc":
"http://npm.lwcjs.org/eslint-plugin-lwc/-/eslint-plugin-lwc-0.4.0/643f5e18247ca8c0a60bf972f4d3795c97e8e6fe.tgz",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rsalvador - Due to possibly, jdx/npm-register#129, the npm.lwcjs.org registry is not properly sending back the packages for @babel and @types. Thus, instead of using the package that way, we embed the tar.url.

This was done by getting the dist.tarball using, e.g., npm view --registry=https://npm.lwcjs.org/ [email protected]. The tar.gz URL option is supported through https://docs.npmjs.com/files/package.json#urls-as-dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you are back from the weekend, it might be worthwhile to see if we want to use https://docs.npmjs.com/files/package.json#bundleddependencies to the lwc-language-server so that we can do it without specifying all its dependencies chain. I think this is a consequence of the refactor that was going on. [email protected] didn't need all these other dependencies brought in.

Choose a reason for hiding this comment

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

Ok, will try that when publishing the next lwc-language-server version

"eslint-plugin-lwc":
"http://npm.lwcjs.org/eslint-plugin-lwc/-/eslint-plugin-lwc-0.4.0/643f5e18247ca8c0a60bf972f4d3795c97e8e6fe.tgz",
"lwc-language-server":
"http://npm.lwcjs.org/lwc-language-server/-/lwc-language-server-1.6.1/48af811b08f90d445c0a5185071129d29df73d39.tgz",
Copy link
Contributor

Choose a reason for hiding this comment

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

We were using version 1.6.6 and this tarball is 1.6.1, any reason for not using 1.6.6 tarball ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. No this is a typo when I was copy-and-pasting. We should use 1.6.6. Let me fix this.

@codecov
Copy link

codecov bot commented Aug 31, 2018

Codecov Report

Merging #611 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #611   +/-   ##
========================================
  Coverage    74.84%   74.84%           
========================================
  Files          158      158           
  Lines         6246     6246           
  Branches       966      966           
========================================
  Hits          4675     4675           
  Misses        1332     1332           
  Partials       239      239

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 560f8c4...6860b0b. Read the comment docs.

@vazexqi vazexqi merged commit 5e06bc6 into develop Aug 31, 2018
@vazexqi vazexqi deleted the nick/use-main-npm-registry branch August 31, 2018 19:21
@dgautsch
Copy link

@vazexqi we found a fix for jdx/npm-register#129 in this PR jdx/npm-register#130

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.

4 participants