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

FAST component observable doesn't work in create-react-app (Typescript) #4503

Closed
anjulgarg opened this issue Mar 26, 2021 · 16 comments · Fixed by #5482
Closed

FAST component observable doesn't work in create-react-app (Typescript) #4503

anjulgarg opened this issue Mar 26, 2021 · 16 comments · Fixed by #5482
Assignees
Labels
community:request Issues specifically reported by a member of the community. docs:article Additions, improvements, or fixes to site articles improvement A non-feature-adding improvement status:planned Work is planned

Comments

@anjulgarg
Copy link

anjulgarg commented Mar 26, 2021

Unable to make an observable re-render the template inside a typescript react app.
(This might be a dumb question OR I might be missing something since I am fairly new to web components.)

"react": "^17.0.2"
"@microsoft/fast-components": "^1.19.1"
"@microsoft/fast-element": "^1.0.1"

Steps to Reproduce

import React from 'react';
import './App.css';
import { FastCounter } from './FastCounter';

FastCounter;

function App() {
  return (
    <div className="App">
      <fast-counter />
    </div>
  );
}

export default App;
import { customElement, FASTElement, html, observable } from '@microsoft/fast-element';

const template = html<FastCounter>`
  <h1>${x => x.counter}</h1>
  <button @click=${x => x.upCount()}>Increase Count</button>
`;

@customElement({name: 'fast-counter', template})
export class FastCounter extends FASTElement {
  @observable counter = 0;

  upCount() {
    console.log('Counter was called');
    this.counter += 1;
    console.log('New count is', this.counter);
  }
}

Screen Shot 2021-03-25 at 10 09 26 PM

Clicking the Button should ideally increase the value from 0 to 1 and so on.

  • OS & Device: MacOS
  • Browser Google Chrome
@anjulgarg anjulgarg added the status:triage New Issue - needs triage label Mar 26, 2021
@EisenbergEffect
Copy link
Contributor

This is extremely odd. Can you share your project by any chance?

@anjulgarg
Copy link
Author

Don't really have a project I can share. But I tried it multiple times and was able to replicate the issue every time.

@nicholasrice
Copy link
Contributor

@anjulgarg is this using create-react-app? There are known integration issues with that package and property decorators.

@anjulgarg
Copy link
Author

anjulgarg commented Mar 29, 2021

Yes. I used the guide here.
https://www.fast.design/docs/integrations/react

@nicholasrice
Copy link
Contributor

Under the hood observable uses the get and set accessors to track mutations and retrievals for the property value. The Babel integration that create-react-app has clobbers those accessors when it defines the property for the class. Even the TypeScript CRA runs TS files through Babel . Last I tried I was unable to configure CRA Babel to not clobber property decorators, and I had to configure webpack to use ts-loader instead.

I'm going to update the docs for this scenario, because https://www.fast.design/docs/integrations/react/#create-react-app doesn't address the TS integration issues.

@anjulgarg
Copy link
Author

anjulgarg commented Mar 29, 2021

Thanks for that info @nicholasrice
I will try setting up react from scratch, and see if the issue goes away.

@anjulgarg
Copy link
Author

@nicholasrice I can confirm that decorators work when I created a react typescript from scratch and setup a custom webpack configuration.
It would be great if we can have this explicitly mentioned in FAST docs. :)

@EisenbergEffect EisenbergEffect added community:request Issues specifically reported by a member of the community. docs:article Additions, improvements, or fixes to site articles improvement A non-feature-adding improvement status:planned Work is planned and removed status:triage New Issue - needs triage labels Apr 5, 2021
@EisenbergEffect
Copy link
Contributor

Turned into a docs request and assigned to @nicholasrice

@nicholasrice nicholasrice changed the title FAST component observable doesn't work in React App (Typescript) FAST component observable doesn't work in create-react-app (Typescript) Apr 5, 2021
@ashwinGokhale
Copy link

I'm also having the same issue 😢. Is there any way to make it work with Babel + Typescript?

@ashwinGokhale
Copy link

To give some more info, with legacy Babel decorators, the code compiles fine, but the decorators don't work because of how they are compiled as stated above ^:

"plugins": [
        "babel-plugin-transform-typescript-metadata",
        ["@babel/plugin-proposal-decorators", { "legacy": true }],
        ["@babel/plugin-proposal-class-properties", { "loose": true }]
    ]

When enabling the decoratorsBeforeExport option, the code compiles, but at runtime it throws an error:

"plugins": [
        "babel-plugin-transform-typescript-metadata",
        ["@babel/plugin-proposal-decorators", { "legacy": false, "decoratorsBeforeExport": true }],
        ["@babel/plugin-proposal-class-properties", { "loose": true}]
    ]

Error thrown at runtime:

Uncaught TypeError: An element descriptor's .kind property must be either "method" or "field", but a decorator created an element descriptor with .kind "undefined"

@ashwinGokhale
Copy link

Did some digging.

These are the Babel plugins I am using:

"plugins": [
        "babel-plugin-transform-typescript-metadata",
        ["@babel/plugin-proposal-decorators", { "legacy": false, "decoratorsBeforeExport": true }],
        ["@babel/plugin-proposal-class-properties", { "loose": true}]
    ]

I think I found a bug in the way the Observable defines properties:
https://github.com/microsoft/fast/blob/master/packages/web-components/fast-element/src/observation/observable.ts#L148

The way that Babel transpiles the code, the parameters will be this:
image

The target will be a Descriptor and the nameOrAccessor will be undefined

The attr decorator on the other hand I don't know about. It is throwing a different runtime error:
image

@EisenbergEffect
Copy link
Contributor

It looks like you may be using the wrong version of decorators. There is no kind property, for example. That was part of the second proposal. There have been 5 or 6 proposals over the years. TS uses the very first proposal and has stayed there awaiting the stabilization of the language feature (which finally looks like it's happening after many years of work). I don't know the correct configuration of the plugin, but I do know there is a way to configure it. @nicholasrice may be able to provide the specifics.

@ashwinGokhale
Copy link

ashwinGokhale commented Apr 23, 2021

I switched back to Babel legacy decorators:

"plugins": [
        "babel-plugin-transform-typescript-metadata",
        ["@babel/plugin-proposal-decorators", { "legacy": true }],
        ["@babel/plugin-proposal-class-properties", { "loose": true }]
    ]

I was able to find a workaround by creating my own observable decorator to get the FastCounter example to work:

function observable<T>(target: T, name: keyof T, p?: any): any {
    const descriptor = {
        set: function (value: T) {
            Observable.notify(this, name);
            (this as any)[`__${name}`] = value;
        },
        get: function () {
            Observable.track(this, name as string);
            return (this as any)[`__${name}`];
        },
        enumerable: true
    };

    Object.defineProperty(target, name, descriptor);

    if (p?.initializer) {
        target[name] = p.initializer();
    }

    return descriptor;
}

@ashwinGokhale
Copy link

Also possibly related, the attr decorator does not trigger rerenders when an attribute changes, but using the attributes array in the customElement decorator instead does work. @nicholasrice @EisenbergEffect Since I was able to get around the observable decorator issue, I feel like this might be a bug related to the implementation of the decorators.

@EisenbergEffect
Copy link
Contributor

@nicholasrice Can you take a few minutes this week to confirm that we've got this resolved with the appropriate update in our documentation? I'd like to close this out if the work's done or if not get a solution in place.

@nicholasrice
Copy link
Contributor

We did, though I'm going to update it to reference this issue for future reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community:request Issues specifically reported by a member of the community. docs:article Additions, improvements, or fixes to site articles improvement A non-feature-adding improvement status:planned Work is planned
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants