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.

Sign up for GitHub

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

Jump to bottom

Get web-build-tools using PNPM #383

Merged
merged 50 commits into from
Jan 9, 2018
Merged

Get web-build-tools using PNPM #383

merged 50 commits into from
Jan 9, 2018

Conversation

nickpape
Copy link
Contributor

No description provided.

@HipsterZipster
Copy link

HipsterZipster commented Oct 3, 2017 •
edited by nickpape
Loading

love this!!! #Resolved

@HipsterZipster
Copy link

HipsterZipster commented Oct 3, 2017 •
edited by nickpape
Loading

Looked at the PR, is pnpmVersion already supported in the rush.json for consumers that are using Rush without the gulp build? I haven't seen any documentation about it
#Resolved

@octogonz octogonz mentioned this pull request Oct 27, 2017
Closed
@nickpape nickpape changed the title Update web-build-tools for Rush+PNPM Have web-build-tools support PNPM Nov 30, 2017
@nickpape
Copy link
Contributor Author

nickpape commented Dec 1, 2017 •
edited
Loading

Hi @HipsterZipster , no "pnpmVersion" was added with #449

You should be able to try it now, although there a number of known issues that we are working on resolving the pnpm maintainer. #Resolved

octogonz
octogonz reviewed Jan 5, 2018
View reviewed changes
rush.json Outdated
@@ -1,5 +1,5 @@
{
"npmVersion": "4.5.0",
"pnpmVersion": "1.25.1",
"rushVersion": "4.1.0",
"nodeSupportedVersionRange": ">=6.9.0 <7.0.0",
Copy link
Collaborator

@octogonz octogonz Jan 5, 2018 •
edited by nickpape
Loading

Choose a reason for hiding this comment

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

"nodeSupportedVersionRange": ">=6.9.0 <7.0.0", [](start = 1, length = 47)

Your PR adds typings for Node 8 #Resolved

@iclanton
Copy link
Member

iclanton commented Jan 5, 2018 •
edited by nickpape
Loading

{

Revert? #Resolved

Refers to: .vscode/launch.json:1 in e0f6e05. [](commit_id = e0f6e05, deletion_comment = False)

iclanton
iclanton reviewed Jan 5, 2018
View reviewed changes
apps/api-documenter/package.json Outdated
@@ -27,7 +27,7 @@
"@types/colors": "1.1.3",
"@types/fs-extra": "0.0.37",
"@types/js-yaml": "3.9.1",
"@types/node": "6.0.88",
"@types/node": "^8.0.0",
Copy link
Member

@iclanton iclanton Jan 5, 2018 •
edited by nickpape
Loading

Choose a reason for hiding this comment

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

"@types/node": "^8.0.0", [](start = 4, length = 24)

We have to use node 8 with PNPM? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. reverted

In reply to: 159794698 [](ancestors = 159794698)

octogonz
octogonz reviewed Jan 5, 2018
View reviewed changes
common/config/rush/nonbrowser-approved-packages.json Outdated
{
"name": "chalk",
"allowedCategories": [ "libraries" ]
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use colors, not chalk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we use chalk in GCB apparently, however it was not defined as a dependency. PNPM exposed this as a problem. I can replace chalk with colors in this PR, or I can also do it in another PR. your call

In reply to: 159794773 [](ancestors = 159794773)

Copy link
Collaborator

Choose a reason for hiding this comment

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

File an issue, use a separate PR

In reply to: 159795444 [](ancestors = 159795444,159794773)

iclanton
iclanton reviewed Jan 5, 2018
View reviewed changes
common/config/rush/pnpmfile.js Outdated
pkg.dependencies['slash'] = '^1.0.0';
}
return pkg;
}
Copy link
Member

@iclanton iclanton Jan 5, 2018 •
edited by nickpape
Loading

Choose a reason for hiding this comment

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

What is this? Does every project that uses PNPM need this file? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a special override for "bad packages" (e.g. jest-runtime didn't add slash as a dependency). There is a long discussion about this file here pnpm/pnpm#949

Goal here is to get WBT working with PNPM before making additional changes to pnpm

In reply to: 159794827 [](ancestors = 159794827)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested and we don't need this since newer version of jest-runtime arent missing the dependency

In reply to: 159795139 [](ancestors = 159795139,159794827)

octogonz
octogonz reviewed Jan 5, 2018
View reviewed changes
core-build/gulp-core-build/src/GulpProxy.ts Outdated
/**
* A helper utility for gulp which can be extended to provide additional features to gulp vinyl streams
*/
export class GulpProxy extends Orchestrator implements gulp.Gulp {
Copy link
Collaborator

@octogonz octogonz Jan 5, 2018 •
edited by nickpape
Loading

Choose a reason for hiding this comment

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

GulpProxy [](start = 13, length = 9)

What is this change?
#Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed gulpproxy because it was causing a lot of issues with node/gulp typings, but I may be able to revert this as well

In reply to: 159794855 [](ancestors = 159794855)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this can also be reverted due to the deduping change

In reply to: 159795595 [](ancestors = 159795595,159794855)

@iclanton
Copy link
Member

iclanton commented Jan 5, 2018 •
edited by nickpape
Loading

export function _isJestEnabled(rootFolder: string): boolean;

Unrelated changes in these files? #Resolved

Refers to: common/reviews/api/gulp-core-build.api.ts:2 in e0f6e05. [](commit_id = e0f6e05, deletion_comment = False)

@nickpape
Copy link
Contributor Author

nickpape commented Jan 5, 2018

export function _isJestEnabled(rootFolder: string): boolean;

Hmm, we may not need the gulp proxy change. It was caused by conflicts in the gulp/node typings. I may be able to revert it. Let me see

In reply to: 355447824 [](ancestors = 355447824)

Refers to: common/reviews/api/gulp-core-build.api.ts:2 in e0f6e05. [](commit_id = e0f6e05, deletion_comment = False)

iclanton
iclanton requested changes Jan 5, 2018
View reviewed changes
Copy link
Member

@iclanton iclanton left a comment

Choose a reason for hiding this comment

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

🕐

iclanton
iclanton approved these changes Jan 5, 2018
View reviewed changes
Copy link
Member

@iclanton iclanton left a comment

Choose a reason for hiding this comment

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

:shipit:

octogonz
octogonz reviewed Jan 5, 2018
View reviewed changes
core-build/gulp-core-build/src/tasks/GulpTask.ts Outdated
@@ -145,7 +144,7 @@ export abstract class GulpTask<TTaskConfig> implements IExecutable {
* @returns a Promise, a Stream or undefined if completeCallback() is called
*/
public abstract executeTask(
gulp: gulp.Gulp | GulpProxy,
Copy link
Collaborator

@octogonz octogonz Jan 5, 2018 •
edited by nickpape
Loading

Choose a reason for hiding this comment

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

p: gulp.Gulp | GulpProxy, [](start = 7, length = 25)

Are you removing GulpProxy? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just missed this change

In reply to: 159797130 [](ancestors = 159797130)

octogonz
octogonz approved these changes Jan 5, 2018
View reviewed changes
Copy link
Collaborator

@octogonz octogonz left a comment

Choose a reason for hiding this comment

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

:shipit:

@nickpape nickpape closed this Jan 5, 2018
@nickpape nickpape reopened this Jan 5, 2018
@octogonz
Copy link
Collaborator

octogonz commented Jan 5, 2018

Fix your PR description

@nickpape nickpape merged commit d00b654 into master Jan 9, 2018
@octogonz
Copy link
Collaborator

octogonz commented Jan 9, 2018

@zkochan FYI

We've now officially converted our first Rush monorepo to use pnpm! In the coming weeks we'll be working on migrating other repos to use this tool. Thanks again for all your help and fixes!

(And great job @nickpape-msft !)

@zkochan
Copy link
Contributor

zkochan commented Jan 9, 2018

This is awesome news! 🎉🍾🎆

@iclanton iclanton deleted the nickpape/test-pnpm branch January 26, 2018 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iclanton iclanton iclanton approved these changes

@octogonz octogonz octogonz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nickpape @HipsterZipster @iclanton @octogonz @zkochan