POC: TS path aliases, standard tools without abstractions, hierarchical storybook#16706
POC: TS path aliases, standard tools without abstractions, hierarchical storybook#16706Hotell wants to merge 3 commits intomicrosoft:masterfrom
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 9470cd2:
|
JustSlone
left a comment
JustSlone
left a comment
There was a problem hiding this comment.
Looking promising 👍
Left some comments / questions / suggestions. I also hit a few issues and mentioned them (I know it's a DRAFT pr, so maybe these are known issues).
| "test": "just-scripts test", | ||
| "update-api": "just-scripts update-api", | ||
| "update-snapshots": "just-scripts jest -u" | ||
| "build": "echo 'TODO: rollup & tsc --declaration && api-extractor'", |
There was a problem hiding this comment.
Is rollup required for the proposed build? (Seems like the RFC says either could work, but I wanted to confim)
David and I had been speculating in passing about switching to rollup, however, lots of tooling kind of assumes webpack, so we might lose some benefit from the larger ecosystem webpack has. Also we've been looking at Module Federation which is a feature of webpack. Lastly, all the apps we work with across MSFT will certainly be using webpack, so that's a factor to consider. These are not necessarily blockers, but factors to consider in the decision.
I guess the point is, that Webpack v rollup seems like a decision with a lot of implications, so it might be easier to stay with webpack for now.
However, I could also see and would support the argument that a great way to learn more would be to try out rollup with the converged build. As long as we can switch back to webpack easily if needed.
What do you or others think we should do?
Also, is there any possibility we could maybe just jump to esbuild? 😄
There was a problem hiding this comment.
Is rollup required for the proposed build?
Not really (just an example) It doesn't matter that much what is used (webpack/rollup) from consumer point of view.
Rollup is common choice in OSS for design systems/libraries, webpack rather for application development.
- it's "faster" (especially with our new hero :D https://github.com/egoist/rollup-plugin-esbuild#readme)
- rollup-ing doesn't add webpack runtime baggage
- check this google tool for better comparison https://bundlers.tooling.report/
Lastly, all the apps we work with across MSFT will certainly be using webpack
...
that Webpack v rollup seems like a decision with a lot of implications
That's fine, I don't see correlation with what is being used outside this repo. We provide design system for consumers that anyone can use without tooling restrains. If we provide tooling as well, I'd like to see the requirements for that to better understand the situation.
Module Federation
Right, need to take a look how much proprietary it is ( if it needs special runtime or not )
Also, is there any possibility we could maybe just jump to esbuild? 😄
That would be awesome, but we need to test it (there might be some glitches)
There was a problem hiding this comment.
I like Rollup, it's awesome, I would say that it's brilliant for small libraries 💎
But, I would be careful with Rollup taking in account that Terser on consumer's side often does not perform well. If we will bundle with Rollup we will get a single file with all possible side-effects.
Real example 1: downshift
I can dig dipper to remember, but functions in this file (https://github.com/downshift-js/downshift/blob/a13308910282c44ea2ebdcbc8a6dfee901df48a2/src/set-a11y-status.js#L26) are producing side-effects that will stay anyway in a bundle.
Real example 2: non obvious Terser behavior
It's a recent discovery from Fluent's code, I have already shared this case with @dzearing. Constants in this file are well defined, what can go wrong? The first line... 💣
const toObjectMap = (...items: (string[] | Record<string, number>)[]) => {};
// will become in JS
var toObjectMap = function () {
var items = [];
for (var _i = 0; _i < arguments.length; _i++) {
items[_i] = arguments[_i];
}
};Still looks good? But it will not be removed by Terser (v5.5.1) ever:
Input
var toObjectMap = function () {
var items = [];
for (var _i = 0; _i < arguments.length; _i++) {
items[_i] = arguments[_i];
}
};
var fooItems = toObjectMap()Rollup 2.18.2
!function(){for(var n=[],f=0;f<arguments.length;f++)n[f]=arguments[f]}();Webpack 5.19.0
BTW, try to find a difference 😏
!function(){for(var n=[],f=0;f<arguments.length;f++)n[f]=arguments[f]}();There was a problem hiding this comment.
Clearly a larger discussion 😉. My comment is resolved as long as nothing in the PR here depends on going one way or the other.
There was a problem hiding this comment.
@layershifter interesting. well those are not constants by definition (as it's dynamic behaviour as you pointed out) - maybe I'm missing something, but that function is needed for runtime, if it would be removed, those constants wouldn't work.
There was a problem hiding this comment.
discussed the issue with @layershifter offline 💪 .
In short: There might be an issue within Terser -> property assignments are preventing tree shaking (array indexes, static props) etc
| "update-snapshots": "just-scripts jest -u" | ||
| "build": "echo 'TODO: rollup & tsc --declaration && api-extractor'", | ||
| "clean": "rm -rf dist lib-{amd,commonjs,es-2015} .cache", | ||
| "start": "yarn serve", |
There was a problem hiding this comment.
I'm guessing this won't be an issue long term, but I thought I would call it out.
I'm having to build @fluentui/react in order to get yarn workspace @fluentui/react-button start to work. This appears to be due to dependencies on @fluentui/react-internal. Will this go away once we clean up react-button to not depend on react-internal and only depend on converged utilities? I assume that cross package dependencies on converged utilities will work without build.
Repro:
- yarn scrub (or just cleaning react-internal will probably repro)
- yarn
- yarn workspace @fluentui/react-button start
Error (many errors of this kind)
ERROR in ../react-internal/src/Icons.ts
Module not found: Error: Can't resolve '@fluentui/font-icons-mdl2' in '/Users/jslone/repos/hotell-fluentui/packages/react-internal/src'
@ ../react-internal/src/Icons.ts 6:9-45
another example:
ERROR in ../react/src/index.ts
Module not found: Error: Can't resolve '@fluentui/react-date-time' in '/Users/jslone/repos/hotell-fluentui/packages/react/src'
@ ../react/src/index.ts 13:9-45
@ ./src/components/Button/Converged.stories.tsx
There was a problem hiding this comment.
I suspect that isn't expected to work yet. Also a bit surprised yarn workspace @fluentui/react-button start even works since react-button is now excluded from the workspace config.
There was a problem hiding this comment.
is now excluded from the workspace config
It's not. That's a leftover a forgot to remove, sorry about that :) .
You cannot exclude from workspace definition only the other way -> yarnpkg/yarn#7051 (comment)
There was a problem hiding this comment.
I assume that cross package dependencies on converged utilities will work without build.
yup.
Why you getting those errors right now:
- I didn't update whole dependency tree to use TS path aliases,
start(storybook) follows "standard webpack" package resolution process and tries to resolve"module"field which points to a location that doesn't exist (no build run).
There was a problem hiding this comment.
Got it, that's what I assumed. So once the whole dependency tree (of converged components and utilities) uses TS path aliases this will work?
Is there something else we have to do to fix the problem where storybook resolves the "module" field?
There was a problem hiding this comment.
Is there something else we have to do to fix the problem where storybook resolves the "module" field?
I guess we need to remove those hardcoded fields and generate them only during build/publish (or point them to source instead of lib)
| const onColorChange = React.useCallback((ev: never, newColor: IColor) => setColor(newColor.str), []); | ||
|
|
||
| return ( | ||
| <Stack gap={16}> |
There was a problem hiding this comment.
It seems like having stories in the react-button package would mean that using components like Stack or Text here would introduce a dependency between react-button and react-stack/react-text. Won't this cause problems?
I know Elizabeth has mentioned this likely being a problem, she might have more details.
That being said, maybe it's a crazy idea, but if we could avoid/solve the dependency problem (or maybe it's not a problem?). Then packages export their stories, which would make pulling examples from our components and community work nicely in the future. I like the idea of a component package (or set of packages) being self contained, should lead to better mix and matching of components in the future.
There was a problem hiding this comment.
Justin is right, the reason we split the stories/examples into a separate package is because having them in the component packages can create circular dependency issues. It doesn't happen in this case because react-button currently depends on react-internal, but it becomes a big issue once you remove the react-internal dep. The cases he mentioned are good examples.
The possibilities I can think of to address the circular dep issue are:
- Put all the examples/stories in a single package, like we have today (downside: not ideal dev experience)
- For each component package, make a corresponding examples/stories package which can depend on anything (downside: each separate package adds maintenance and build overhead)
- Separate the concepts of dev-time examples (stories?) and documentation examples. Dev-time examples live in component packages (and can't depend on other components, at least not in circular ways) and documentation examples live in a single separate package. Historically we've used the same examples for development and documentation (IIRC in both v8 and v0) but maybe we should reconsider.
That being said, maybe it's a crazy idea, but if we could avoid/solve the dependency problem (or maybe it's not a problem?). Then packages export their stories, which would make pulling examples from our components and community work nicely in the future. I like the idea of a component package (or set of packages) being self contained, should lead to better mix and matching of components in the future.
I think the only way to keep the stories in the same package and avoid the dependency issue would be option 3 above, and it would be dev-time stories only. But even if we did that, I don't think packages should export (or probably even publish) stories. Certainly shouldn't export them by default since that would lead to a confusing dev experience.
This actually reminds me of another minor reason that we put the examples in a separate package: so they could theoretically be consumed in other published packages without inflating the size of the main package. In the past, when our examples lived in OUFR, they were also published with OUFR (because the website package uses the examples, and in the past the website package was published and partially reused by an internal version of the site). This led to complaints from some partners (ODSP mainly) that our package was too big. While this wasn't the main motivation for moving the examples, it didn't hurt.
I'd like to say that now that the website package isn't published, it would be possible to have examples in the component package (ignoring cycle concerns for the moment) and just exclude them from publishing using npmignore. However David mentioned on Friday that he may have to start publishing the website package again (something related to the module federation work??)...at which point we'd be stuck publishing everything it depends on too, including examples.
There was a problem hiding this comment.
Yeah, maybe I'm over complicating things with the idea of exporting stories. Perhaps we should just go with a simple setup. Or more likely this is a problem to be solved through whatever storybook setup we go with.
There was a problem hiding this comment.
+1 on the circular dependencies problem.
I think that it is better to have just one set of examples for all - dev, docs and screenshot tests.
Also as we plan to provide other teams with FUI storybook with all components, I would prefer to have just one storybook for all components instead of having per-component storybooks.
cc @levithomason as this is closely related/overlaps with his current work
There was a problem hiding this comment.
circular deps:
First of all, thanks a lot for thorough background regarding this topic, which brings me back to "crucial need for requirements" :). if someone can handle us such a document, I guess we would save a lot of time.
- If we gonna use storybook, there is no such a thing as circular deps, as SB will traverse paths and bundle only files we define to bundle (
.stories.*in this case) to create an App not a package. This fits right in with what is @levithomason trying to achieve (SB composition from various repos). - exporting stories as public API/package ? This doesn't make any sense to me TBH (from storybook API point of view - your tool queries for what it needs to build )
e2e coverage: from my experience e2e should be completely separated from implementation
package + stories-> storybook app available at urlpackage-e2e-> separate folder/package (not publishable) contains only test suites that access storybook via url, thus testing it end to end
Also as we plan to provide other teams with FUI storybook with all components, I would prefer to have just one storybook for all components instead of having per-component storybooks.
This covers both scenarios.
- Ship+build all to one storybook/ship+build only per package.
- Sidenote: from my experience it is rather annoying if one needs to build whole suite to be able to work on 1 component.
There was a problem hiding this comment.
OK, this should be simple :) Seems we all agree the "ideal" scenario is to colocate stories with components for optimal dev experience and to ship those examples to the docs for our users.
Martin, show us no circular dependencies 👍 I believe you can get a specific example of react-button circular deps from Mak as well.
There was a problem hiding this comment.
- If we gonna use storybook, there is no such a thing as circular deps, as SB will traverse paths and bundle only files we define to bundle (
.stories.*in this case) to create an App not a package. This fits right in with what is @levithomason trying to achieve (SB composition from various repos).
I'm confused...how does just the fact of using storybook mean there are suddenly no circular deps? Would story files in individual component packages be excluded from the package's build and tsconfig? Where in the build process would they get type checked (important to prevent stories' API usage from getting out of date)?
I'd also appreciate more background from @levithomason about this proposal of multi-repo storybook, especially whether it's also intended to be used as the inner loop for this repo (which wouldn't make sense to me...seems way too heavy-weight for inner dev loop).
| </ThemeProvider> | ||
| ); | ||
|
|
||
| export const ThemeExample = () => { |
There was a problem hiding this comment.
Maybe it's something wrong on my end, but intellisense is not working for me in VSCode on Button in the stories (either existing or new).
example, this results in no suggestions, even when forcing Vscode to provide them
<Button onCli
There was a problem hiding this comment.
good catch! I'm afraid this rabbit hole goes quite deep and is probably out of scope of this POC.
Why it doesn't work?
TL;DR:
- problem is with
React.forwardRef, which defines our button api in incorrectlyButton(wondering if this new config exposed a significant bug or if it ever worked for consumers ? )
Extended explanation:
- so
.forwardRefhas following definition:
function forwardRef<T, P = {}>(
render: ForwardRefRenderFunction<T, P>
): ForwardRefExoticComponent<PropsWithoutRef<P> & RefAttributes<T>>;
-
the issue starts here ->
PropsWithoutRef<P>- which is is mapped type defined as:
'ref' extends keyof P ? Pick<P, Exclude<keyof P, 'ref'>> : P; -
now if we pass
ButtonPropstoPropsWithoutRef:
->PropsWithoutRef<ButtonProps>
-> we'll get following{[x: string]: any; [x: number]: any;}🚨
-> huh what? - wellkeyof ButtonProps === string | number🚨🚨 -
in the end what we get is
ForwardRefExoticComponent<{[x: string]: any; [x: number]: any;} & RefAttributes<HTMLElement>>signature
-> which results to:(props: {🚨[x: string]: any; 🚨[x: number]: any; ref?: HTMLElement; key?: string | number}) => (ReactElement|null);
Summary:
💡those index types break intellisense/type checking (props: {🚨[x: string]: any; 🚨[x: number]:any )
There was a problem hiding this comment.
Mystery solved:
The root cause of aforementioned problem lies within ButtonProps definition.
While the definition might look all good ->
export type ButtonProps = ComponentProps & React.HTMLAttributes<HTMLElement> & {...}If we take closer look at ComponentProps, we'll get:
-> interface ComponentProps extends GenericDictionary {}
going further down the inheritance chain we found the root cause -> GenericDictionary
export type GenericDictionary = Record<string, any>;
using indexed keyed dictionaries creates not very solid API surface and with any it's very dangerous to use. In this scenario it gets even worse by how it resolves when we wanna use keyof operator.
Demo:
- if we remove
GenericDictionaryfrom the chain everything works as expected
- export interface ComponentProps extends GenericDictionary {...}
+ export interface ComponentProps {...}
There was a problem hiding this comment.
nice investigation! 🔥
Doesn't seem like extending GenericDictionary is a good idea. Should we just fix this in button right now? (maybe in a separate PR)
| @@ -0,0 +1,90 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Today we have separate examples that are converted into stories and also used as examples on the website. Is there a clean way to reuse some of these stories as examples on the website? I would really rather not have two separate systems for code examples.
There was a problem hiding this comment.
I don't think any of the stories are new files, just copied, but you bring up an interesting question.
For old packages we've used the same *.Example.tsx files for both development and documentation (website), but we don't appear to have a clear policy for how converged components should handle this yet. Today the "proto-converged" packages have a random mix of *.stories.tsx (usually exporting multiple stories) and *.Example.tsx (usually copied from the v8 component's examples).
More on this in a response to a later comment.
| @@ -0,0 +1,90 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Are we thinking about how to avoid lock in with storybook stories?
We will build a significant collateral of code examples used for inner dev loop, VR/Snapshot tests, and public code examples. I would hate for this collateral to get locked in with storybook, preventing us from (or making it terribly expensive to) migrate to something else in the future.
Are we thinking about this, do we have way to avoid this? I probably doesn't need to be perfect, but we might want to mitigate the degree to which we get locked in.
There was a problem hiding this comment.
I'm not sure we need to worry too much about that right now. Like I mentioned in the previous reply, these files are just copies, so not a change to the status quo. More generally, there's nothing storybook-specific about this format aside from the name (it's just a file exporting a bunch of components), so moving to some other format shouldn't be too hard.
There was a problem hiding this comment.
I would really rather not have two separate systems for code examples.
yeah I fully agree. I just copied button stories from react-examples to be collocated. Ideally react-examples shouldn't exist, or if it does only as an e2e suite aggregator.
how to avoid lock in with storybook stories
Heh but we are locked in already with webpack/jest/react/ ...
What's important is to have solid universal format for "real" docs ( md/mdx ). With that format/metadata we can switch to anything basically (what @ecraig12345 mentioned).
out off topic: from what I heard the plan is to have 1 storybook to aggregate all components out there from various repos
so not a change to the status quo.
From my experience, consistency should be primary goal (simplicity,Dx...). With aforementioned setup, we are not consistent - eg. we have test collocated with implementation but docs live somewhere completely different . TBH I don't see any benefit in that.
There was a problem hiding this comment.
From my experience, consistency should be primary goal (simplicity,Dx...). With aforementioned setup, we are not consistent - eg. we have test collocated with implementation but docs live somewhere completely different . TBH I don't see any benefit in that.
The only way we could have docs live in the same package is if we can find a way around the circular dependency issues mentioned elsewhere--which would be nice but seems unlikely.
ecraig12345
left a comment
ecraig12345
left a comment
There was a problem hiding this comment.
Like Justin said, this could be promising but I have some questions.
| testMatch: ['**/+(*.)+(spec|test).+(ts)?(x)'], | ||
| testURL: 'http://localhost', | ||
| moduleNameMapper, | ||
| cacheDirectory: '<rootDir>/.cache/jest', |
There was a problem hiding this comment.
👍 we should probably add a similar setting to our current package configuration--having jest cache in its default location outside the repo is counterintuitive
There was a problem hiding this comment.
agree, I'd suggest to do this for every tool that we use and has caching capabilities (eslint, storybook...)
| "root": true, | ||
| "overrides": [ | ||
| { | ||
| "files": ["*.stories.{ts,tsx}"], |
There was a problem hiding this comment.
If we go forward with this change or similar, this rule should move to the overrides in @fluentui/eslint-plugin/react.
Also a more general comment, I have a hard time understanding the value of lint rules with "warning" level. If people decide to disregard the warning, it either causes spam in logs (annoying and potentially confusing) or requires adding a disable comment (same as would happen with an error level rule that someone felt should be ignored in a particular spot). However I'm definitely not blocking on this because based on common presets it's clear that a lot of the community doesn't share my opinion.
There was a problem hiding this comment.
Note: This is just a showcase that package/domain specific configuration for linting works as expected
I have a hard time understanding the value of lint rules with "warning" level
I'm not getting into this type of conversation as is out of scope of what I'm trying to demonstrate here.
short comment though :): from my experience, errors in linters are unnecessary noise that distracts from delivering real value. Severity error makes only sense if the rule has no Fixer and if violation of that rule would introduce:
- security risk
- measurable perf degradation
- runtime issue
eg. in this repo: having line longer that 120 is error, that's quite bad DX from my exp.
There was a problem hiding this comment.
Note: This is just a showcase that package/domain specific configuration for linting works as expected
Got it, that makes sense.
I am actually interested in discussing general ideas of how linting ought to work some other time and considering whether our current approach is actually the right one (that was kind of why I mentioned the error thing), but agreed that it's not important to discuss now and not blocking this PR.
| "update-snapshots": "just-scripts jest -u" | ||
| "build": "echo 'TODO: rollup & tsc --declaration && api-extractor'", | ||
| "clean": "rm -rf dist lib-{amd,commonjs,es-2015} .cache", | ||
| "start": "yarn serve", |
There was a problem hiding this comment.
I suspect that isn't expected to work yet. Also a bit surprised yarn workspace @fluentui/react-button start even works since react-button is now excluded from the workspace config.
| "serve": "start-storybook --ci -c ./.storybook -p 6006", | ||
| "test": "jest", | ||
| "lint": "eslint --ext=.ts,.tsx src/", | ||
| "format": "prettier **/*.{ts,tsx,md,html,scss} --write", |
There was a problem hiding this comment.
One concern I have with duplicating the configuration for each command in every package.json is that it would have a negative impact on maintainability (if the options need to be changed for some reason, as far as I can tell you'd have to rely on find/replace and hope it caught everywhere). What has been your experience with that in other projects?
There was a problem hiding this comment.
Note: this is a POC :)
What has been your experience with that in other projects?
It depends on requirements which to be honest I've been missing lately 😅.
In general there are 2 common options (from my experience):
- evolutionary architecture (used mainly for application development in monorepo )
- meaning: you provide common defaults, but it's up to the team/domain owner to tweak as needed
- 1 way how to do things (used mainly for narrower scope - like design system )
- meaning: you provide common defaults that are enforced everywhere
- mixture of both
a negative impact on maintainability
Also most of the time there is this very thin line between how differentiate DRY/Hard to maintain.
There was a problem hiding this comment.
Got it, I wasn't clear on whether the manual configured commands in each package.json were intended as part of the proposal, or just a way to make things work right now (sounds like it's the latter, so I'm not too worried about it for now).
There was a problem hiding this comment.
yup your latter statement. thx
| @@ -0,0 +1,90 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
I don't think any of the stories are new files, just copied, but you bring up an interesting question.
For old packages we've used the same *.Example.tsx files for both development and documentation (website), but we don't appear to have a clear policy for how converged components should handle this yet. Today the "proto-converged" packages have a random mix of *.stories.tsx (usually exporting multiple stories) and *.Example.tsx (usually copied from the v8 component's examples).
More on this in a response to a later comment.
| const onColorChange = React.useCallback((ev: never, newColor: IColor) => setColor(newColor.str), []); | ||
|
|
||
| return ( | ||
| <Stack gap={16}> |
There was a problem hiding this comment.
Justin is right, the reason we split the stories/examples into a separate package is because having them in the component packages can create circular dependency issues. It doesn't happen in this case because react-button currently depends on react-internal, but it becomes a big issue once you remove the react-internal dep. The cases he mentioned are good examples.
The possibilities I can think of to address the circular dep issue are:
- Put all the examples/stories in a single package, like we have today (downside: not ideal dev experience)
- For each component package, make a corresponding examples/stories package which can depend on anything (downside: each separate package adds maintenance and build overhead)
- Separate the concepts of dev-time examples (stories?) and documentation examples. Dev-time examples live in component packages (and can't depend on other components, at least not in circular ways) and documentation examples live in a single separate package. Historically we've used the same examples for development and documentation (IIRC in both v8 and v0) but maybe we should reconsider.
That being said, maybe it's a crazy idea, but if we could avoid/solve the dependency problem (or maybe it's not a problem?). Then packages export their stories, which would make pulling examples from our components and community work nicely in the future. I like the idea of a component package (or set of packages) being self contained, should lead to better mix and matching of components in the future.
I think the only way to keep the stories in the same package and avoid the dependency issue would be option 3 above, and it would be dev-time stories only. But even if we did that, I don't think packages should export (or probably even publish) stories. Certainly shouldn't export them by default since that would lead to a confusing dev experience.
This actually reminds me of another minor reason that we put the examples in a separate package: so they could theoretically be consumed in other published packages without inflating the size of the main package. In the past, when our examples lived in OUFR, they were also published with OUFR (because the website package uses the examples, and in the past the website package was published and partially reused by an internal version of the site). This led to complaints from some partners (ODSP mainly) that our package was too big. While this wasn't the main motivation for moving the examples, it didn't hurt.
I'd like to say that now that the website package isn't published, it would be possible to have examples in the component package (ignoring cycle concerns for the moment) and just exclude them from publishing using npmignore. However David mentioned on Friday that he may have to start publishing the website package again (something related to the module federation work??)...at which point we'd be stuck publishing everything it depends on too, including examples.
| "@fluentui/eslint-plugin": "^1.0.0-beta.0", | ||
| "@types/enzyme": "3.10.3", | ||
| "@types/enzyme-adapter-react-16": "1.0.3", | ||
| "@types/jest": "~24.9.0", |
There was a problem hiding this comment.
I think removing this may cause errors from the import/no-extraneous-dependencies rule.
There was a problem hiding this comment.
with my proposal that lint rule should not be needed :) (out of scope, but Im aware of that, thx)
There was a problem hiding this comment.
The lint rule would still be pretty important for preventing implicit deps in production code, but I see how it would become less important for tests.
| "baseUrl": ".", | ||
| "rootDir": ".", | ||
| "paths": { | ||
| "@fluentui/react-button": ["packages/react-button/src/index.ts"], |
There was a problem hiding this comment.
Can this be shorted to "@fluentui/*": ["packages/*/src/index.ts"], with overrides for cases where that path is incorrect? tsconfig.fluentui.json does something similar today.
There was a problem hiding this comment.
yeah it can but it doesn't conform requirements of my proposal:
- will we able to use this for convergence only? - no
besides that
- is it explicit ? - not really
| "target": "ES2015", | ||
| "lib": ["es2017", "dom"], |
There was a problem hiding this comment.
Unless we plan to start requiring consumers to use polyfills, we should only include up to es5 to ensure IE 11 support. (We most likely can't build our code itself with polyfills included due to bundle size concerns.)
There was a problem hiding this comment.
lib specification from TypeScript is not accurate btw - for that reason, babel should be used to properly transpile code based on browsers we want support, thus not blocking us to not be able to use latest ECMAscript spec.
Unless we plan to start requiring consumers to use polyfills, we should only include up to es5 to ensure IE 11 support.
Do we have somewhere proper set of requirements what is needed/ what we plan to support for convergence ?
There was a problem hiding this comment.
lib specification from TypeScript is not accurate btw - for that reason, babel should be used to properly transpile code based on browsers we want support, thus not blocking us to not be able to use latest ECMAscript spec.
What do you mean by not accurate? I know IE 11 supports a few things beyond what lib: es5 includes, but for v7/8 it's generally been an adequate heuristic.
Unless we plan to start requiring consumers to use polyfills, we should only include up to es5 to ensure IE 11 support.
Do we have somewhere proper set of requirements what is needed/ what we plan to support for convergence ?
This has been the perpetual headache of the past year: people from one team have one set of requirements in their minds, and the other team has a slightly different set, and often nobody notices until pretty late (when we've already wasted months of work) because nobody wrote the requirements down.
In this case, I'm not 100% clear on what the requirements are since I've only been on the edge of convergence discussions. I posted about it in the convergence channel and will try to get someone who's more closely involved to write it down.
There was a problem hiding this comment.
What do you mean by not accurate?
babel can apply transpilation set by proper browser support restrictions. In case of ie 11, es5 is too strict and low level -> thus increasing bundle size/perf etc. If we setup browser restrictions accordingly we can produce more concise code to be shipped to consumers.
| "update-snapshots": "just-scripts jest -u" | ||
| "build": "echo 'TODO: rollup & tsc --declaration && api-extractor'", | ||
| "clean": "rm -rf dist lib-{amd,commonjs,es-2015} .cache", | ||
| "start": "yarn serve", |
There was a problem hiding this comment.
is now excluded from the workspace config
It's not. That's a leftover a forgot to remove, sorry about that :) .
You cannot exclude from workspace definition only the other way -> yarnpkg/yarn#7051 (comment)
| "update-snapshots": "just-scripts jest -u" | ||
| "build": "echo 'TODO: rollup & tsc --declaration && api-extractor'", | ||
| "clean": "rm -rf dist lib-{amd,commonjs,es-2015} .cache", | ||
| "start": "yarn serve", |
There was a problem hiding this comment.
I assume that cross package dependencies on converged utilities will work without build.
yup.
Why you getting those errors right now:
- I didn't update whole dependency tree to use TS path aliases,
start(storybook) follows "standard webpack" package resolution process and tries to resolve"module"field which points to a location that doesn't exist (no build run).
| "serve": "start-storybook --ci -c ./.storybook -p 6006", | ||
| "test": "jest", | ||
| "lint": "eslint --ext=.ts,.tsx src/", | ||
| "format": "prettier **/*.{ts,tsx,md,html,scss} --write", |
There was a problem hiding this comment.
Note: this is a POC :)
What has been your experience with that in other projects?
It depends on requirements which to be honest I've been missing lately 😅.
In general there are 2 common options (from my experience):
- evolutionary architecture (used mainly for application development in monorepo )
- meaning: you provide common defaults, but it's up to the team/domain owner to tweak as needed
- 1 way how to do things (used mainly for narrower scope - like design system )
- meaning: you provide common defaults that are enforced everywhere
- mixture of both
a negative impact on maintainability
Also most of the time there is this very thin line between how differentiate DRY/Hard to maintain.
| testMatch: ['**/+(*.)+(spec|test).+(ts)?(x)'], | ||
| testURL: 'http://localhost', | ||
| moduleNameMapper, | ||
| cacheDirectory: '<rootDir>/.cache/jest', |
There was a problem hiding this comment.
agree, I'd suggest to do this for every tool that we use and has caching capabilities (eslint, storybook...)
| "@fluentui/eslint-plugin": "^1.0.0-beta.0", | ||
| "@types/enzyme": "3.10.3", | ||
| "@types/enzyme-adapter-react-16": "1.0.3", | ||
| "@types/jest": "~24.9.0", |
There was a problem hiding this comment.
with my proposal that lint rule should not be needed :) (out of scope, but Im aware of that, thx)
| "target": "ES2015", | ||
| "lib": ["es2017", "dom"], |
There was a problem hiding this comment.
lib specification from TypeScript is not accurate btw - for that reason, babel should be used to properly transpile code based on browsers we want support, thus not blocking us to not be able to use latest ECMAscript spec.
Unless we plan to start requiring consumers to use polyfills, we should only include up to es5 to ensure IE 11 support.
Do we have somewhere proper set of requirements what is needed/ what we plan to support for convergence ?
| "baseUrl": ".", | ||
| "rootDir": ".", | ||
| "paths": { | ||
| "@fluentui/react-button": ["packages/react-button/src/index.ts"], |
There was a problem hiding this comment.
yeah it can but it doesn't conform requirements of my proposal:
- will we able to use this for convergence only? - no
besides that
- is it explicit ? - not really
| test: /\.(ts|tsx)$/, | ||
| use: [ | ||
| { | ||
| loader: require.resolve('ts-loader'), |
There was a problem hiding this comment.
You mentioned in proposal that we will use Babel for complication, do you plan to use babel-loader instead? 🐱
There was a problem hiding this comment.
Oops, it seems that I see the plan few lines below 💭
There was a problem hiding this comment.
yes, I had various issues incorporating it into this existing component, (as react-internal is using decorators/namespaces/const enums - "blockers" for babel), so I didn't wanted to spend a lot of time on it.
| ['@babel/plugin-proposal-decorators', { legacy: true }], | ||
| '@babel/plugin-proposal-object-rest-spread', | ||
| '@babel/plugin-proposal-class-properties', | ||
| '@babel/plugin-syntax-dynamic-import', |
There was a problem hiding this comment.
Are you going to extract this to .babelrc/babel.config.js? It would be nice to put .browserlistrc
There was a problem hiding this comment.
yes and yes ( lack of time - POC )
| transform: { | ||
| '^.+\\.[tj]sx?$': ['babel-jest', { cwd: __dirname, configFile: './babel-jest.config.json' }], | ||
| }, | ||
| moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx'], |
There was a problem hiding this comment.
| transform: { | |
| '^.+\\.[tj]sx?$': ['babel-jest', { cwd: __dirname, configFile: './babel-jest.config.json' }], | |
| }, | |
| moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx'], | |
| transform: { | |
| '^.+\\.tsx?$': ['babel-jest', { cwd: __dirname, configFile: './babel-jest.config.json' }], | |
| }, | |
| moduleFileExtensions: ['ts', 'tsx'], |
nit: I don't think that we plan to we have .js/.jsx files in repo as test targets
6 participants
Description of changes
This is POC based on #16570
react-button( non converged component ), just to showcase the architectureArchitecture (simplified)
New Dev workflow (per package):
Things that are far from perfect (slow in this POC):
Tests:
isConformantcalls are extremely slow, especially on cold start - caused mainly by react-doc block