chore(react-menu): implement docs generation with ts path aliases #17449
Conversation
Hotell
changed the title
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7fcac66:
|
| @@ -84,9 +84,13 @@ | |||
| "@types/prettier": "1.19.1", | |||
| "@types/webpack-dev-middleware": "4.1.0", | |||
| "@types/webpack-env": "1.16.0", | |||
| "@microsoft/api-extractor": "7.7.1", | |||
| "@types/chalk": "2.2.0", | |||
| "@types/yargs": "13.0.11", | |||
There was a problem hiding this comment.
we are now explicit instead using this as a transient dependency
| * | ||
| * @param {string[]} files | ||
| */ | ||
| function normalizeImports(files) { |
There was a problem hiding this comment.
I know this is very naive, but I don't think we need to go AST way for such a simple task :)
There was a problem hiding this comment.
I am just thankful that our folder names reflec the package name... otherwise this might have been a nightmare
There was a problem hiding this comment.
Can you comment somewhere that this just assumes the above fact and just replaces relative . dots with the @fluentui/scope
There was a problem hiding this comment.
hmm is that needed? I mean that's the only issue why is this needed - local package deps, besides that TS works as expected.
Also there are tests :)
There was a problem hiding this comment.
It's true that the tests help with documentation, but it wouldn't hurt to mention the reason inline as well for anyone who comes across this later and isn't familiar with the whole scenario.
Hotell
force-pushed
the
hotell/16889/api-extractor-with-ts-path
branch
from
1bdbe63 to
ee0a007
Compare
Hotell
requested review from
assuncaocharles,
chrisdholt,
ecraig12345,
EisenbergEffect,
jurokapsiar,
layershifter,
ling1726,
miroslavstastny,
nicholasrice and
pompomon
as code owners
| @@ -0,0 +1,91 @@ | |||
| // @ts-check | |||
There was a problem hiding this comment.
nit: the other tests in the package use .test as an extension, might be good to be consistent
There was a problem hiding this comment.
right.
I created RFC to start using proper naming suffix - .spec, as we are using behaviour driven approach. Once we agree upon consistency we can re-name in batch and add proper automation checks.
| @@ -11,6 +11,8 @@ | |||
| }, | |||
| "license": "MIT", | |||
| "scripts": { | |||
| "docs": "api-extractor run --config=config/api-extractor.local.json --local", | |||
| "build:local": "tsc -p . --module esnext --emitDeclarationOnly && node config/normalize-import --output dist/react-menu/src && yarn docs", | |||
| "build": "just-scripts build", | |||
There was a problem hiding this comment.
I'm curious, so on CI, the relative type imports isn't an issue ?
There was a problem hiding this comment.
that's correct. on CI lage takes the job of tsc dep resolution when running builds ordered by dep tree relationships.
There was a problem hiding this comment.
more info documented in JSDoc + TS issue
Can we you please clarify when we should use |
currently this is a temporary placeholder (following what folks from web-components team do) so we don't have inlined the whole command within `"build:local" script -> calling it directly would throw error if one did any public api changes in particular package (because ts output was not re-generated), thus should not be used directly. again this is a temporary solution until we migrate all converged packages to this new stack and enable it on CI as well to to unify things. maybe we can name it as |
| return filePath; | ||
| }); | ||
|
|
||
| return allFiles.flat(Infinity); |
There was a problem hiding this comment.
Using this means we've implicitly dropped support for Node 10 (that's why the lib reference was needed). Practically speaking that's probably okay at this point (given that it will be officially unsupported after next month) but I'd prefer to do it separately with a more intentional/coordinated approach: updating the @types/node version, updating docs, and announcing to contributors.
There was a problem hiding this comment.
- AFAIK we support node 12 and up
- there are no restrictions on node on yarn set by tooling (missing engines field completely) ( CI uses 12 :) )
- @ling1726 what node are you using (as you're the only consumer for now)?
We should address all of this in different PR/sprint
There was a problem hiding this comment.
version 12, as it says in our contributing guide
There was a problem hiding this comment.
and here's what is written in our official setup guide
There was a problem hiding this comment.
The "verify your environment" section goes into more detail about current requirements.
node -v: Should be 12.x.x or 14.x.x (where "x" is some number). 10.x should still work for now. Newer versions may not work.
Again, I don't think that dropping 10 is a problem at this point, I would just prefer for it to happen in a more coordinated manner.
| * @type {Console['log']} | ||
| */ | ||
| // eslint-disable-next-line no-console | ||
| const log = console.log.bind(console); |
There was a problem hiding this comment.
What's the motivation for doing this, rather than using console.log directly and disabling the lint rule for the file? (edit: or add an override to disable no-console for the config directory)
There was a problem hiding this comment.
- a good rule of thumb is to not turn off rules for whole file
logis shorter - instead of having console.log all over the place- ideally we should use robust and common logging solution - like debug
There was a problem hiding this comment.
I see nothing wrong with turning off a rule which is clearly irrelevant for a given file (or ideally disabling this rule for all config files). Making an alias for a common method decreases clarity and is a micro-optimization, and IMO not really worth it especially if your main goal is just to get around a lint rule.
There was a problem hiding this comment.
The intent of the no-console rule is to minimize console logs in component/other browser code files. It shouldn't be enabled for config files to begin with. So the best short-term workaround (either in this PR or a follow-up) would be to update the shared eslint config to ensure that it uses appropriate rules on config files. Agreed that a proper logger would be nice but that's an entirely separate problem.
Not quite following here--the |
Hotell
changed the title
updated description a bit |
The PR description update helps, thanks. (Previously I wasn't clear on where this command fit into the workflow, and the suggestion of "verify" made it seem like there might be some misunderstanding of what the API Extractor command would do.)
|
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: d471952c96f813c4faa1a9b721c908f155b53e4a (build) |
There was a problem hiding this comment.
I made a PR #17594 fixing the build issue with API Extractor.
Re: #17449 (comment) I'd probably prefer update-api as the name for the new command since it follows a convention at least people from v7/8 are used to (conventions from web-components are less relevant due to lack of overlap) but would also be okay with sticking with build:local.
I'd still very strongly prefer not to implicitly drop Node 10 (explicitly dropping it by updating our types version and docs would be fine), but not going to block on it.
|
@chrisdholt or @EisenbergEffect can you take a quick look and sign off on the changes to the web-components files? |
@EisenbergEffect API Extractor is pretty tightly tied to a certain TS version generally, so @joschect will be upgrading it for most of the repo as part of the upgrade to TS 4.1 that he's working on. However we probably won't be changing the TS version for web-components (it's up to you all when to do that), so if the new API Extractor version doesn't work with the current TS version in web-components, it may be necessary to add back a separate API Extractor version for just web-components and let you upgrade it later. |
|
@ecraig12345 Just as long as we have that open for the future since the new API extractor features will greatly improve our doc generation capabilities. |
Hotell
force-pushed
the
hotell/16889/api-extractor-with-ts-path
branch
from
ee0a007 to
7fcac66
Compare
|
🎉 Handy links: |
… local build (microsoft#17449) * chore: move chalk,api-extractor,yargs,tmp to single version policy * chore(react-menu): implement docs generation without ts path aliases * Change files
Pull request checklist
$ yarn changeDescription of changes
This is a temporary workaround to enable full dev workflow with no manual build needed (via lage --to) when working on convergence packages.
Before:
lage built --to @fluentui/package-nameand wait for api.md changes (same as on CI)After:
yarn workspace @fluentui/package-name build:localFocus areas to test
(optional)