-
Notifications
You must be signed in to change notification settings - Fork 22
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
build(root): Initial project scaffolding #2
Conversation
add single quote pref to editor config
@salesforce/sa11y-reviewers Can you give a quick review and merge this please. This PR does not contain any code changes. It is initial setup and config (mostly inspired from Salesforce, LWC projects) that is in its own PR to help make the changes in subsequent PRs easier to review. |
# Conflicts: # yarn.lock
@mohanraj-r Will take a look at this soon. |
@@ -0,0 +1,2 @@ | |||
@types:registry=https://registry.npmjs.org/ | |||
registry=https://registry.npmjs.org/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to set these? They should be the defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I have the internal nexus registry configured in the global .npmrc
, it was resulting in refs to the internal nexus in the yarn.lock
file. Hence had to override it to make sure there are no refs to internal registry - which could affect the CI among others. I guess most projects override the publish config in package.json
to the public npm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the references should point to the pubic registry rather than internal Nexus for this project. From an open source point of view I don't think there should be any references to the public registry though. I personally comment out the line pointing to Nexus in my root npmrc and only uncomment it when I need to access internal stuff.
Since it's probably a ways away from being truly open source it's probably fine to leave for now, but I think it should be removed by the time it's exposed externally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we can also switch npm registries using profiles https://docs.npmjs.com/configuring-your-registry-settings-as-an-npm-enterprise-user#create-a-profile-for-the-public-npm-registry
But then this adds a manual step to the process which could be easily forgotten.
Agree that we can revisit this later again when we are ready to make this public.
Readme.md
Outdated
@@ -0,0 +1,24 @@ | |||
# Salesforce Accessibility Automation Libraries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this file name is usually all caps (README.md
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
*/ | ||
module.exports = { | ||
extends: ['@commitlint/config-conventional'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally find that linting commits like this can be confusing for people who don't use it frequently. I've also seen people be confused by what exactly the issue is with their commit title/message.
Nothing wrong with keeping this if you like it, but if you do I would make sure when there's an error it's clear what's wrong. LWC has a section in the CONTRIBUTING.md
that details their specific commit rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know thanks. Added contribution guidelines to this project as well based on lwc.
Besides from uniformity in commit msgs, the main benefit I am hoping for is in auto-generating changelog. Other than that I am not sure I see a clear benefit for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw I am also going to try limiting to squashing commits when merging to master - to have a saner commit history (since I tend to create a lot of granular commits). So this should result in one commit per PR in master in which case the commit linting is rendered moot I guess. If this could be problematic or not ideal, please let me know.
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
*/ | ||
module.exports = { | ||
preset: 'ts-jest', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Jest 24 there's much better support for Typescript: https://jestjs.io/blog/2019/01/25/jest-24-refreshing-polished-typescript-friendly#typescript-support.
The maintainer of ts-jest has basically said to just use native Jest: kulshekhar/ts-jest#961 (comment)
I wouldn't say this is a must-have, but worth considering for the future since other projects have reported a speed improvement moving away from ts-jest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw references to using babel for compiling ts. But then I also saw https://kulshekhar.github.io/ts-jest/user/babel7-or-ts which lists some features not available in babel yet. Not sure if that info is current. And since this project doesn't use babel yet - I didn't want to bring it in as a dependency yet. Not sure if we will need babel for any other purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if you'll need Babel for other stuff. FWIW Babel is a dependency of Jest so you technically are already pulling it in to the project as a transitive dependency.
I think using babel + Jest is the best thing to do but I don't think it will be particularly hard to make that switch in the future so if you want to stick with this for now and re-evaluate later when you have more tests that's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Trevor. Didn't know that babel is already a Jest dependency. Tried replacing ts-jest with babel/preset-typescript
as per https://jestjs.io/docs/en/getting-started. Ran into some errors with jest trying to run the .js
files as well as .d.ts
files in addition to .ts
files. Probably needs more config than what is mentioned in those docs. Also with the caveats https://babeljs.io/docs/en/next/babel-plugin-transform-typescript.html#caveats tempted to give this a try again a little later.
- uses: actions/checkout@v2 | ||
- uses: actions/setup-node@v1 | ||
with: | ||
node-version: '10.x' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Active LTS support for Node 10 ends the end of April. Version 12 is active LTS until the end of October, it may be worth switching to 12 now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for the info! Updated.
with: | ||
node-version: '10.x' | ||
- run: yarn install | ||
- run: yarn test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a linting job here before unit tests?
- run: yarn lint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Had it in the #3 PR - ported it over. Made a yarn ci
target so as to make it easier to manage.
packages/rules/package.json
Outdated
@@ -0,0 +1,21 @@ | |||
{ | |||
"name": "@sa11y/rules", | |||
"version": "1.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would default versions to 0.1.0
. Having the major version set signals that the API is set. Having the major version at 0 signals it's still under active development and things can change at any time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, updated!
@mohanraj-r Left a few comments. I don't think anything is a blocker so if you want feel free to merge now. Otherwise I can re-review after you make changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for the review @trevor-bliss
Have made changes and comments. Please review.
@@ -0,0 +1,2 @@ | |||
@types:registry=https://registry.npmjs.org/ | |||
registry=https://registry.npmjs.org/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I have the internal nexus registry configured in the global .npmrc
, it was resulting in refs to the internal nexus in the yarn.lock
file. Hence had to override it to make sure there are no refs to internal registry - which could affect the CI among others. I guess most projects override the publish config in package.json
to the public npm?
Readme.md
Outdated
@@ -0,0 +1,24 @@ | |||
# Salesforce Accessibility Automation Libraries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
*/ | ||
module.exports = { | ||
extends: ['@commitlint/config-conventional'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know thanks. Added contribution guidelines to this project as well based on lwc.
Besides from uniformity in commit msgs, the main benefit I am hoping for is in auto-generating changelog. Other than that I am not sure I see a clear benefit for now.
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
*/ | ||
module.exports = { | ||
preset: 'ts-jest', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw references to using babel for compiling ts. But then I also saw https://kulshekhar.github.io/ts-jest/user/babel7-or-ts which lists some features not available in babel yet. Not sure if that info is current. And since this project doesn't use babel yet - I didn't want to bring it in as a dependency yet. Not sure if we will need babel for any other purpose?
- uses: actions/checkout@v2 | ||
- uses: actions/setup-node@v1 | ||
with: | ||
node-version: '10.x' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for the info! Updated.
with: | ||
node-version: '10.x' | ||
- run: yarn install | ||
- run: yarn test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Had it in the #3 PR - ported it over. Made a yarn ci
target so as to make it easier to manage.
packages/rules/package.json
Outdated
@@ -0,0 +1,21 @@ | |||
{ | |||
"name": "@sa11y/rules", | |||
"version": "1.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, updated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review and approval @trevor-bliss
Looks like I can't merge it myself - seeing "Merging is blocked" on my side. Probably because of a github protected branch config limiting authors from merging - not sure. Can you please merge it if you can.
@@ -0,0 +1,2 @@ | |||
@types:registry=https://registry.npmjs.org/ | |||
registry=https://registry.npmjs.org/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we can also switch npm registries using profiles https://docs.npmjs.com/configuring-your-registry-settings-as-an-npm-enterprise-user#create-a-profile-for-the-public-npm-registry
But then this adds a manual step to the process which could be easily forgotten.
Agree that we can revisit this later again when we are ready to make this public.
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
*/ | ||
module.exports = { | ||
extends: ['@commitlint/config-conventional'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw I am also going to try limiting to squashing commits when merging to master - to have a saner commit history (since I tend to create a lot of granular commits). So this should result in one commit per PR in master in which case the commit linting is rendered moot I guess. If this could be problematic or not ideal, please let me know.
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
*/ | ||
module.exports = { | ||
preset: 'ts-jest', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Trevor. Didn't know that babel is already a Jest dependency. Tried replacing ts-jest with babel/preset-typescript
as per https://jestjs.io/docs/en/getting-started. Ran into some errors with jest trying to run the .js
files as well as .d.ts
files in addition to .ts
files. Probably needs more config than what is mentioned in those docs. Also with the caveats https://babeljs.io/docs/en/next/babel-plugin-transform-typescript.html#caveats tempted to give this a try again a little later.
Yes, merging is blocked because of the protected branch settings in the repo. I cannot merge either. I also see this error here: "At least 1 approving review is required by reviewers with write access". I see in the sa11y-reviewers group I'm a part of, I have "Triage" permissions, but the branch protection rules require someone with write access to approve. If you're a maintainer/admin of this repo I recommend going to the Settings on the repo home page and either loosening the branch protection rules so you can still merge, or giving me write permissions to satisfy the branch protections. |
Thanks @trevor-bliss - Changed sa11y-reviewers to have write perms. When I saw that "Triage" said "Can also manage issues and pull requests" I thought that would include merging pull requests. Sorry for the confusion. |
@trevor-bliss Not sure if we need to "Require signed commits" for an OSS project? Have to figure out how to do that for multiple github accounts - not able to find a way similar to using multiple ssh keys. |
@mohanraj-r Looks like commit signing was turned off. I think this is good to merge now. Feel free to merge whenever you're ready. |
Thanks for the review and approvals @trevor-bliss @cordeliadillon . Merged. |
Setup initial scaffolding for monorepo with required dev deps (based on lwc projects). This PR should be merged before others.