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.

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

fix(react): remove attribute when is "false" #190

Merged
merged 16 commits into from
Jul 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Meta } from '@storybook/addon-docs'

<Meta title="Docs/Architecture Decision Records/ADR 0008: Why do we need Grids" />

# Why do we need Grids
# ADR 0008: Why do we need Grids

🗓️ 2023-05 · ✍️ [@mauriciomutte](https://twitter.com/mauriciomutte) and [@felipefialho](https://twitter.com/felipefialho_)

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { Meta } from '@storybook/addon-docs'

<Meta title="Docs/Architecture Decision Records/ADR 0010: Why do we need a fix in React?" />

# ADR 0010: Why do we need a fix in React?

🗓️ 2023-07 · ✍️ [@igorwessel](https://twitter.com/igor_wessel)

## Context

In order to achieve a better DX with boolean props in React.

## Problems

Today (07/19/2023) the Stencil does not follow the HTML spec for boolean props, which caused problems in testing/styling because expected to not have a attribute in HTML when is false.

Example:

```jsx

<AtomButton disabled={false} />

// output in html

<atom-button disabled="false" />
```

So it affects styles, and tests where the boolean attribute shouldn't exist

Examples:

```css
// regardless if disabled is false/true will still be applied.
atom-button[disabled] {
background-color: red;
}
```

```js
// regardless if disabled is false/true will throw a false-positive because expects the attribute does not exist.
expect(atom - button).toBeDisabled()
```

## Decision

After we consider the options:

- Living with the problem for possibly in the near future react will offer better [support](https://github.com/facebook/react/issues/11347) for web-components.
> We can see [tests in a experimental branch here](https://custom-elements-everywhere.com/libraries/react-experimental/results/results.html)
- We are already transpiling the web-component to React, could use a workaround.

Based on these options, we decide to use workaround which will basically set/remove attributes after [occurs a update in component.](https://react.dev/reference/react/Component#componentdidupdate)
1 change: 1 addition & 0 deletions packages/core/jest.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ module.exports = {
coveragePathIgnorePatterns: ['/node_modules/'],
collectCoverageFrom: [
'src/components/**/*.{js,jsx,ts,tsx}',
'output-target/**/*.{js,jsx,ts,tsx}',
'!src/**/stories/**',
'!src/**/*.mock.ts',
'!src/**/*.spec.ts',
Expand Down
64 changes: 64 additions & 0 deletions packages/core/output-target/react-boolean.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import {
Compiler,
CompilerSystem,
createCompiler,
loadConfig,
} from '@stencil/core/compiler'
import { createNodeLogger, createNodeSys } from '@stencil/core/sys/node'
import { reactOutputTarget } from '@stencil/react-output-target'
import fs from 'fs'
import { reactBooleanFixOutputTarget } from './react-boolean'

describe('React Boolean Fix', () => {
let compiler: Compiler
let sys: CompilerSystem

beforeAll(async () => {
// https://stenciljs.com/docs/compiler-api#createcompiler
const logger = createNodeLogger({ process })
sys = createNodeSys({ process })

const validated = await loadConfig({
logger,
sys,
config: {
_isTesting: true,
outputTargets: [
reactOutputTarget({
// We are informing a file that does not exist to create the react-library inside that folder.
proxiesFile: './output-target/index.ts',
}),
reactBooleanFixOutputTarget({
attachPropsFile: './react-component-lib/utils/attachProps.ts',
}),
],
},
})

compiler = await createCompiler(validated.config)
})

afterAll(async () => {
await fs.promises.rm('./output-target/react-component-lib', {
recursive: true,
})
await compiler.destroy()
})

it('should apply fix correctly in react library', async () => {
jest
.spyOn(sys, 'writeFile')
.mockImplementation(jest.fn().mockResolvedValue(true))

const fsSpyOn = jest
.spyOn(fs.promises, 'writeFile')
.mockImplementation(jest.fn().mockResolvedValue(true))

await compiler.build()

expect(fsSpyOn).toHaveBeenCalledWith(
expect.any(String),
expect.stringContaining("if (propType === 'boolean')")
)
})
})
79 changes: 79 additions & 0 deletions packages/core/output-target/react-boolean.ts
igorwessel marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/**
* You can read about the need for this file at ADR.
* Link: https://juntossomosmais.github.io/atomium/?path=/docs/docs-architecture-decision-records-adr-0010-why-do-we-need-react-fix--docs
*/

import {
igorwessel marked this conversation as resolved.
Show resolved Hide resolved
BuildCtx,
CompilerCtx,
Config,
OutputTargetCustom,
} from '@stencil/core/internal'
import fs from 'fs'
import path from 'path'

interface IReactBooleanOutputTargetOptions {
attachPropsFile: string
}

const messages = {
start: 'Generating fix for react boolean',
finish: 'Generate fix for react boolean finished',
}

const runReactBooleanFixOutputTarget = async (
outputTarget: IReactBooleanOutputTargetOptions
) => {
const attachPropsFilePath = path.resolve(
__dirname,
outputTarget.attachPropsFile
)

let attachPropsContent = await fs.promises.readFile(attachPropsFilePath, {
encoding: 'utf-8',
})

attachPropsContent = attachPropsContent.replace(
"if (propType === 'string') {",
`if (propType === 'boolean') {
if (newProps[name] === true) {
node.setAttribute(camelToDashCase(name), '');
} else {
node.removeAttribute(camelToDashCase(name));
}
} else if (propType === 'string') {`
)

await fs.promises.writeFile(attachPropsFilePath, attachPropsContent)
}

export const reactBooleanFixOutputTarget = (
outputTarget: IReactBooleanOutputTargetOptions
): OutputTargetCustom => ({
type: 'custom',
name: 'react-boolean-fix',
generator: async (
_config: Config,
compilerCtx: CompilerCtx,
buildCtx: BuildCtx
) => {
const timespan = buildCtx.createTimeSpan(messages.start, true)

await new Promise((resolve) => {
compilerCtx.events.on('buildLog', (log) => {
const isFinishedTranspileReact =
log.messages.findIndex((message) =>
message.includes('generate react-library finished')
) !== -1

if (isFinishedTranspileReact) {
resolve(true)
}
})
})

await runReactBooleanFixOutputTarget(outputTarget)

timespan.finish(messages.finish)
},
})
17 changes: 10 additions & 7 deletions packages/core/sonar-project.properties
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,23 @@ sonar.test.inclusions=\
src/**/*.spec.js,\
src/**/*.spec.jsx,\
src/**/*.spec.ts,\
src/**/*.spec.tsx
src/**/*.spec.tsx,\
output-target/*.spec.ts
sonar.coverage.exclusions=\
src/components/grid/col/col.tsx
src/components/grid/col/col.tsx\
output-target/*.spec.ts,\
sonar.cpd.exclusions=\
src/components/textarea/**
sonar.exclusions=\
node_modules/**,\
**/dist/**,\
.eslintrc.js,\
jest.config.js,\
src/*.spec.js,\
src/*.spec.jsx,\
src/*.spec.ts,\
src/*.spec.tsx,\
src/**/*.spec.js,\
src/**/*.spec.jsx,\
src/**/*.spec.ts,\
src/**/*.spec.tsx,\
output-target/*.spec.ts,\
.md,\
*.mdx,\
src/**/stories/**,\
Expand All @@ -42,4 +45,4 @@ src/typings.d.ts,\
src/index.ts,\

sonar.typescript.tsconfigPath=tsconfig.json
sonar.javascript.lcov.reportPaths=coverage/lcov.info
sonar.javascript.lcov.reportPaths=coverage/lcov.info
5 changes: 5 additions & 0 deletions packages/core/stencil.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { sass } from '@stencil/sass'

import { reactOutputTarget } from '@stencil/react-output-target'
import { vueOutputTarget } from '@stencil/vue-output-target'
import { reactBooleanFixOutputTarget } from './output-target/react-boolean'

export const config: Config = {
namespace: 'core',
Expand Down Expand Up @@ -123,6 +124,10 @@ export const config: Config = {
'ion-toolbar',
],
}),
reactBooleanFixOutputTarget({
attachPropsFile:
'../../react/src/components/react-component-lib/utils/attachProps.ts',
}),
{
type: 'dist',
esmLoaderPath: '../loader',
Expand Down