Added the default FAST project template package

[janechu]

Pull Request

📖 Description

This adds a FAST project template package to be used by the upcoming FAST CLI. It contains:

• A webpack setup
• TypeScript
• Playwright testing

🎫 Issues

Resolves microsoft/fast-cli#1

👩‍💻 Reviewer Notes

Please note that the default web component is added to the page using createElement, this may not be the greatest approach, I'm open to suggestions. It was done in an effort to leverage HTMLWebpackPlugin so that baseline HTML would not be needed to be maintained.

There is also a maintenance concern regarding the duplication between package.json files, this will be addressed in a future PR.

📑 Test Plan

Playwright is the only testing used.

✅ Checklist

General

• I have included a change request file using $ yarn change
• I have added tests for my changes.
• I have tested my changes.
• I have updated the project documentation to reflect my changes.
• I have read the CONTRIBUTING documentation and followed the standards for this project.

⏭ Next Steps

• Add a package to create a project fast-cli#2
• Add a package for the FAST CLI fast-cli#3

[nicholasrice]

Looks good!

[awentzel]

Why are you adding 'npx'? https://github.com/npm/npx (now deprecated and part of NPM)

[janechu]

I am not adding npx, npx comes with npm, that is why the individual package was deprecated. This is how playwright documentation suggests to install the browsers, see https://playwright.dev/docs/intro?msclkid=0d4f729aa6e411ecb1768b44af0fd72e#manually

[radium-v]

yarn playwright install chromium --with-deps should work the same way here

[janechu]

I think this might be preference/opinion due to issue #5329, I'd say for future work not including yarn is inline with our goals outlined in the issue but I can see how using it could also be the case vis a vis consistency with current commands. I'll stick with future-forward position of using npx, unless someone has a big issue with it.

[radium-v]

We have a static version of Yarn 1.22.11 installed to .yarn/releases/yarn-1.22.11.cjs in the repository to avoid potential issues that can arise from developers using different versions of Yarn. This can also help with debugging, since we know that every user (including CI) is using the specific script from the repo and not an alternative package manager.

Fluent uses this baked-in Yarn behavior as well, and even goes further by running the use-yarn-please.js script to prevent developers from using anything other than the locally-scoped version included in their monorepo.

[nicholasrice]

I agree with @janechu here, barring some functional difference I'm not aware of. If we have a long-term goal to migrate away from Yarn, then I think it makes sense to favor npx.

[awentzel]

Greater consistency equals greater maintainability. I'd recommend, sticking with Yarn in this case. If we want to fix it forward, then I'd recommend submitting a new PR to switch everything to NPX. Otherwise, we're unnecessarily fragmenting our codebase with no guarantee future PRs would fix forward.

I agree with the future path noted on #5329 and I believe this is the point at which we should switch away from Lerna to fully leverage npm/npx at one time.

[EisenbergEffect]

We've already moved to an npm-based workflow in the tools repos I believe. It's something I think we want to do with the main monorepo whenever we can manage the logistics of it. I'd prefer to start edging that way if we can. Another option is to move all the CLI stuff to the tools repo.

[radium-v]

I'm fine with switching to NPM, but I'm not 100% sold on using conflicting tools in CI when there's already precedence in our workflows for this exact situation:

fast/.github/workflows/ci-validate-platforms.yml

Line 77 in 082acb9

yarn playwright install

[janechu]

npx does not conflict with yarn, it uses the same method of checking the node_modules folders for the installation before executing it.

[awentzel]

curious about the file name. What is 'cfp'? It's not a common acronym I know. Can we instead use a common industry acronym or not abbreviate for better code maintainability.

[janechu]

It stands for "Create Fast Project", a naming convention I liked from reacts Create React App "cra-template" approach.

[janechu]

I've updated the README to include the acronym.

[awentzel]

My point here is that as a coding best practice, only well-known industry acronyms should be used. Anything else should be spelled out.

Also, all of our previous package names begin with 'fast-'. I don't think we should deviate from this pattern as It helps for discoverability across the web for those that have never heard of FAST. I could also argue, since this is the starter project, perhaps it's intentional to use a unique name to help understand which package is most important when onboarding. In this case, I like using a name that starts with "create-" or "start-" etc. But, in source code view often people navigate folder structures before reading readme's. So spelling the file name out again, has multiple benefits.

[chrisdholt]

^ This is the case with exceptions where using existing convention would cause friction with tools or goals. I think that is an important caveat here. An example - our eslint package name doesn't coincide with the actual package name in package.json, because we went with convention rather than understanding that there is a prescribed naming approach to ensure packages can be used as eslint plugins. I'm all for consistency until it makes meeting our goals more difficult.

I think intent is necessary in understanding the "why" here. @janechu can correct me here, but one big reason to not be verbose is that I can install this template from the command line and verbosity is not our friend in that case. Even in the case that we are invoking this under the covers, a code comment accomplishes the goal, a README helps onboard folks to the project on it's purpose, and I believe that README's are also helpful for search results, etc.

I think that as a rule we should follow conventions for package naming, but there are exceptions - specifically when the name of that package is necessary for meeting specific goals or product outcomes. In that case, like with our eslint package, the convention actually creates more of a mess/disconnect.

[EisenbergEffect]

Agree with Chris and Jane on this. We're following some conventions based on other patterns in CLIs and also don't want this to be too long if it needs to be used on the command-line.

[awentzel]

Blocking mainly on the name and usage of npx. I'd like not to introduce yet another library that's now included with NPM. Also, I would like to make sure the name follows the same patterns we've been using and is intuitive. This helps for discoverability and project maintenance.

Are we using Playwright anywhere else? Will this take over for Storybook eventually?

[janechu]

@awentzel See my replies, npx is shipped with npm. It is not "deprecated", the individual package was moved so the individual implementation was deprecated. npx still has uses, see the playwright install documentation. Playwright is a testing tool, automated, similar to a karma/mocha/chai setup, it will replace that. It does not have anything to do with Storybook which is more of a development environment.

[chrisdholt]

Just one nitpicky preference against enums, so do with that what you will :). This is awesome to see coming together!

Reviewer re-requested but this did not reset the requested changes UI

[EisenbergEffect]

We've already moved to an npm-based workflow in the tools repos I believe. It's something I think we want to do with the main monorepo whenever we can manage the logistics of it. I'd prefer to start edging that way if we can. Another option is to move all the CLI stuff to the tools repo.

[EisenbergEffect]

When I was playing around with things a little while back I was able to use Playwright with Chai. What about using that configuration so it matches the rest of our tests? The setup I used was from modern-web.dev https://modern-web.dev/guides/test-runner/getting-started/

[janechu]

Is that our position for the future when we switch off Karma?

[EisenbergEffect]

Chai is just the test/assertion library so it can be used with various runners. It is my personal preference and what our unit tests are written with (my personal preferences aren't a decided factor of course). I'm not sure if we have an official opinion in the Playwright world yet. I'd like to hear people's opinions though. Maybe the Playwright API is a better approach for tests in general. It does feel more modern in that it uses modules rather than globals. Maybe if we just had a unit test setup with the same library?

[janechu]

I'm fine with doing mocha chai as well since presumably we can run unit tests with just that and also use it for assertion in playwright for homogeny like you mentioned, but I also don't want to unilaterally decide. Let's get opinions from @nicholasrice and @chrisdholt at minimum. Responding here might be best as it's in context.

[nicholasrice]

I took the path of least resistance for the SSR package, which is using expect from Playwright. It has been suitable so far, though I haven't flexed it in that package. I believe under the hood Playright exposes Jest's expect (https://playwright.dev/docs/api/class-test) though, so I expect it is robust. I personally like Jest's expect and find it intuitive, but I can appreciate if it's not everyone's preference. As long as IntiliSense works, I'm comfortable working with any assertion lib.

[EisenbergEffect]

I think we should simplify the initial web component by removing as much of the CSS as possible and getting rid of the theme part. I think that might go against the adaptive UI approach that the rest of the libraries support. No need to complicate the starter template with that IMO.

[EisenbergEffect]

(This is the only real reason for blocking. The rest we can deal with in a future update.)

[janechu]

Well, the only issue with that is it removes the "thing I can write a test against" reasoning for having it in there. Do you have an alternate suggestion? Otherwise I suppose I could remove it and the tests just check for the presence of the component.

[EisenbergEffect]

I wonder if we should have two different test commands, one for unit tests and one for e2e tests and then a command to run them both. Looks like we only have the e2e tests right now. It should be possible to handle both with Playwright. See the link to modern-web.dev below that shows Playwright unit testing with Chai. Not sure how much is required to set this up from scratch without that site's library though. Worth looking into I think.

[janechu]

When/if we get consensus from others on the testing strategy I'll go ahead and implement this change, just waiting on responses.

[janechu]

Closing this PR for now - we will move this package to a new repository specifically for the FAST CLI, where we will also be able to test out playwrights assertion lib vs mocha/chai vis a vis speed and give feedback for future FAST unit & E2E testing decisions.