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

build: add eslint-plugin-node rulesets to yarn lint #1714

Closed
wants to merge 23 commits into from
Closed

build: add eslint-plugin-node rulesets to yarn lint #1714

wants to merge 23 commits into from

Conversation

hailiu2586
Copy link
Contributor

@hailiu2586 hailiu2586 commented Dec 6, 2019 •
edited
Loading

added eslint-plugin-node ruleset to yarn lint

  1. This allows yarn lint to scan for missing entry in package.json from use of 'import ...' in the source code (rule 'node/no-extraneous-import')

  2. fixed the reported rule violations in tools and extensions by looking up the missing entry from yarn.lock and add those the corresponding package.json

  3. added inline suppression of no-extraneous-require for absh specific command

The net result is that 'npm install --production' will work against packages/server/package.json, (after transform '@bfc/...: *' to '@bfc/..: {path to local .tgz}'

refs #1778

Type of change

Please delete options that are not relevant.

  • [x ] New feature (non-breaking change which adds functionality)

Checklist

  • [x ] I have performed a self-review of my own code
  • [x ] I have functionally tested my change

Screenshots

Please include screenshots or gifs if your PR include UX changes.

@hailiu2586 hailiu2586 changed the title Hailiu/server build feat: add .tgz support to prepare-prod.js Dec 6, 2019
@a-b-r-o-w-n a-b-r-o-w-n changed the base branch from stable to master December 9, 2019 20:32
@a-b-r-o-w-n
Copy link
Contributor

a-b-r-o-w-n commented Dec 9, 2019 •
edited
Loading

@hailiu2586 I recently removed the need for prepare-prod.js and instead stopped fighting against yarn workspaces. Checkout the updated Dockerfile if you are interested. That being said, since we want to support the ABS-H deploy, we will be happy to accept some platform changes. I'll need to consider the botbuilder-expression change from a semver to a uri resolution. As for what to do with prepare-prod.js, is it possible for this not to be included in the Composer repo?

EDIT:
I forgot to mention that the botbuilder-expression package should be published to npm as part of the R7 release, so maybe we can hold on that change until that occurs and the problem is solved?

@cwhitten cwhitten mentioned this pull request Dec 10, 2019
Closed
7 tasks
@boydc2014
Copy link
Contributor

@hailiu2586 I recently removed the need for prepare-prod.js and instead stopped fighting against yarn workspaces. Checkout the updated Dockerfile if you are interested. That being said, since we want to support the ABS-H deploy, we will be happy to accept some platform changes. I'll need to consider the botbuilder-expression change from a semver to a uri resolution. As for what to do with prepare-prod.js, is it possible for this not to be included in the Composer repo?

EDIT:
I forgot to mention that the botbuilder-expression package should be published to npm as part of the R7 release, so maybe we can hold on that change until that occurs and the problem is solved?

Yes, expression-library will goes into npm in 1 day or 2. Npm will be used as release version, but i'm expecting composer will somehow still need daily package from myget, and the correct package name for that is "botframework-expression" (so the dependency in indexers needs to be corrected) and the suggested way to refer a daily package, is through yarn.lock, as we did previously, and as Andy copied into docker.

@zhixzhan
Copy link
Contributor

@hailiu2586 I recently removed the need for prepare-prod.js and instead stopped fighting against yarn workspaces. Checkout the updated Dockerfile if you are interested. That being said, since we want to support the ABS-H deploy, we will be happy to accept some platform changes. I'll need to consider the botbuilder-expression change from a semver to a uri resolution. As for what to do with prepare-prod.js, is it possible for this not to be included in the Composer repo?
EDIT:
I forgot to mention that the botbuilder-expression package should be published to npm as part of the R7 release, so maybe we can hold on that change until that occurs and the problem is solved?

Yes, expression-library will goes into npm in 1 day or 2. Npm will be used as release version, but i'm expecting composer will somehow still need daily package from myget, and the correct package name for that is "botframework-expression" (so the dependency in indexers needs to be corrected) and the suggested way to refer a daily package, is through yarn.lock, as we did previously, and as Andy copied into docker.

fixed in #1737

@hailiu2586 hailiu2586 changed the title feat: add .tgz support to prepare-prod.js build: add .tgz support to prepare-prod.js Dec 13, 2019
@github-actions
Copy link

Coverage Status

Coverage decreased (-0.04%) to 40.278% when pulling 30a4984 on hailiu/server-build into 99e87dc on master.

@hailiu2586 hailiu2586 requested a review from yeze322 as a code owner December 16, 2019 18:40
@hailiu2586 hailiu2586 changed the title build: add .tgz support to prepare-prod.js build: add eslint-plugin-node rulesets to yarn lint Dec 16, 2019
a-b-r-o-w-n
a-b-r-o-w-n requested changes Dec 16, 2019
View reviewed changes
Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n left a comment

Choose a reason for hiding this comment

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

Got a couple of minor changes. Can we remove the app insights code from this PR and review that separately?

hailiu2586
hailiu2586 commented Dec 19, 2019
View reviewed changes
Composer/packages/server/src/server.ts
launchLanguageServer(webSocket);
} else {
webSocket.on('open', () => {
configAppInsightsAsNeeded().then(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@a-b-r-o-w-n does this refactor meet your requirement on how app insights get configured?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking we could add that in a separate pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @hailiu2586

@a-b-r-o-w-n a-b-r-o-w-n deleted the hailiu/server-build branch May 18, 2020 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boydc2014 boydc2014 Awaiting requested review from boydc2014 boydc2014 is a code owner

@cwhitten cwhitten Awaiting requested review from cwhitten cwhitten is a code owner

@yeze322 yeze322 Awaiting requested review from yeze322

@a-b-r-o-w-n a-b-r-o-w-n Awaiting requested review from a-b-r-o-w-n a-b-r-o-w-n is a code owner

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
@hailiu2586 @a-b-r-o-w-n @boydc2014 @zhixzhan @cwhitten