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: lwc compiler errors on .jsx/.tsx files #4164

Conversation

SourabhMulay
Copy link
Contributor

Details

Fix: LWC compiler errors on .jsx/.tsx files #4122.
(Treating .jsx/.tsx file extensions the same as .js & .ts.)

Does this pull request introduce a breaking change?

  • 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • 🤞 No, it does not introduce an observable change.

@SourabhMulay SourabhMulay requested a review from a team as a code owner April 20, 2024 07:17
Copy link

Thanks for the contribution! Unfortunately we can't verify the commit author(s): Saurabh Mulay <s***@s***.i***.s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce Inc. Contributor License Agreement and this Pull Request will be revalidated.

@nolanlawson
Copy link
Contributor

Hi @SaurabhMulay999 thank you for the contribution! Could you please add a test to confirm the fix? The right place to add a test would be in transform-javascript.spec.ts. We just need to confirm that the new file extensions are handled correctly.

@SourabhMulay
Copy link
Contributor Author

Hi @SaurabhMulay999 thank you for the contribution! Could you please add a test to confirm the fix? The right place to add a test would be in transform-javascript.spec.ts. We just need to confirm that the new file extensions are handled correctly.

Sure, I'll try.

@SourabhMulay
Copy link
Contributor Author

SourabhMulay commented Apr 23, 2024

Hi @nolanlawson ,
I have tried writing a unit test.

Code:

describe('transformFile function',()=>{
    it('should be tranform Typescript React file (.tsx)',()=>{
        const src=`import React from 'react'; \n const App=()=><div>Hello World</div>;`
        const fileName='app.tsx';
        const options={namespace:'c',name:'app'};
        const result=transformSync(src,fileName,options);
        expect(result.code).toBeDefined();
        expect(result.map).toBeDefined();
        expect(result.warnings).toBeUndefined();
        expect(result.cssScopeTokens).toBeUndefined();

    });

    it('should be tranform Typescript React file (.jsx)',()=>{
        const src=`import React from 'react'; \n const App=()=><div>Hello World</div>;`
        const fileName='app.jsx';

        const options={namespace:'c',name:'app'};
        const result=transformSync(src,fileName,options);
        expect(result.code).toBeDefined();
        expect(result.map).toBeDefined();
        expect(result.warnings).toBeUndefined();
        expect(result.cssScopeTokens).toBeUndefined();

    });
});

after debug it was failing saying that.

Error:
Support for the experimental syntax 'jsx' isn't currently enabled.qq

I have checked some online resources it was suggesting to install @babel/preset-react.

@nolanlawson
Copy link
Contributor

nolanlawson commented Apr 23, 2024

@SaurabhMulay999 It should be fine to just put actual JavaScript in there rather than JSX.

We do not need to test transpiling JSX into JavaScript; just that the .jsx/.tsx extensions are supported.

In a real app setup (e.g. using Rollup), the consumer of LWC would be responsible for ensuring that there are plugins in place to do the transformation.

@SourabhMulay
Copy link
Contributor Author

@SaurabhMulay999 It should be fine to just put actual JavaScript in there rather than JSX.

We do not need to test transpiling JSX into JavaScript; just that the .jsx/.tsx extensions are supported.

In a real app setup (e.g. using Rollup), the consumer of LWC would be responsible for ensuring that there are plugins in place to do the transformation.

Okay. I've added the JS format. I have ran it locally it was successful. Let me know if any modifications are required.

@nolanlawson
Copy link
Contributor

@SaurabhMulay999 Could you please sign the CLA as described above?

import { LightningElement } from 'lwc';
export default class Foo extends LightningElement {}
`;
const fileName = 'app.js';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const fileName = 'app.js';
const fileName = 'app.jsx';

@SourabhMulay
Copy link
Contributor Author

SourabhMulay commented Apr 23, 2024

@SaurabhMulay999 Could you please sign the CLA as described above?

Hey @nolanlawson,
I don't know what is the issue but previously i already have signed the cla. that site redirect me and shows that, that i already have signed the cla. but this time system failed to verify my email. is there any workaround? like to re sign it.

@wjhsf wjhsf closed this Apr 23, 2024
@wjhsf wjhsf reopened this Apr 23, 2024
@wjhsf
Copy link
Contributor

wjhsf commented Apr 23, 2024

Commit authors must be associated with GitHub users

I think you'll have to redo the commits using the email address associated with your GitHub account. (Or add the email address from your commits to your GitHub account, but the one used here doesn't seem to be a real email address, so I don't think GitHub will let you.)

Copy link
Member

@ekashida ekashida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to loop over an array of all the supported file extensions. Something like:

    describe('file extension support', () => {
        function testFileExtensionSupport(ext: string) {
            it(`should support ${ext} file extension`, () => {
                const src = `
                    import { LightningElement } from 'lwc';
                    export default class Foo extends LightningElement {}
                `;
                const options = { namespace: 'c', name: 'foo' };
                const result = transformSync(src, `foo${ext}`, options);
                expect(result.code).toBeDefined();
                expect(result.map).toBeDefined();
                expect(result.warnings).toBeUndefined();
                expect(result.cssScopeTokens).toBeUndefined();
            });
        }
        ['.js', '.jsx', '.ts', '.tsx'].forEach(testFileExtensionSupport);
    });

@@ -269,3 +269,34 @@ describe('sourcemaps', () => {
});
});
});

describe('transformFile function', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
describe('transformFile function', () => {
describe('file extension support', () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better as you have suggested above to loop over all the extensions.

@@ -269,3 +269,34 @@ describe('sourcemaps', () => {
});
});
});

describe('transformFile function', () => {
it('should be tranform Typescript file with ext (.tsx)', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('should be tranform Typescript file with ext (.tsx)', () => {
it('should support .tsx file extension', () => {

expect(result.cssScopeTokens).toBeUndefined();
});

it('should be tranform Javascript file with ext (.jsx)', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('should be tranform Javascript file with ext (.jsx)', () => {
it('should support .jsx file extension', () => {

@SourabhMulay
Copy link
Contributor Author

Commit authors must be associated with GitHub users

I think you'll have to redo the commits using the email address associated with your GitHub account. (Or add the email address from your commits to your GitHub account, but the one used here doesn't seem to be a real email address, so I don't think GitHub will let you.)

I think it tried to fetch my official salesforce email. I'll try adding the personal email and then recommit.

@wjhsf
Copy link
Contributor

wjhsf commented Apr 23, 2024

I think it tried to fetch my official salesforce email. I'll try adding the personal email and then recommit.

If you use your salesforce email then you don't need to sign the CLA, because it will recognize you as an internal user.

@SourabhMulay
Copy link
Contributor Author

I think it tried to fetch my official salesforce email. I'll try adding the personal email and then recommit.
If you use your salesforce email then you don't need to sign the CLA, because it will recognize you as an internal user.

Yeah, but this is my personal account. I have added the Name as well email in the latest commit. It should have now recognise me as a GitHub user.

Copy link
Member

@ekashida ekashida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@nolanlawson
Copy link
Contributor

Yeah, but this is my personal account. I have added the Name as well email in the latest commit. It should have now recognise me as a GitHub user.

The older commits still show the invalid email address. I believe you will need to do an interactive rebase or squash-merge followed by a force-push to fix this.

@SourabhMulay
Copy link
Contributor Author

Yeah, but this is my personal account. I have added the Name as well email in the latest commit. It should have now recognise me as a GitHub user.

The older commits still show the invalid email address. I believe you will need to do an interactive rebase or squash-merge followed by a force-push to fix this.

Okay. I'll try interactive rebase.

@SourabhMulay SourabhMulay force-pushed the Saurabh/Fix-compiler-error-on-jsx/tsx-files branch from b60432e to 84af21a Compare April 24, 2024 04:54
fix:lwc compiler errors on jsx tsx files

fix: lwc compiler errors on jsx tsx files

fix: lwc compiler errors on jsx tsx files

fix: lwc compiler errors on jsx tsx files (unit tests)

fix: lwc compiler errors on jsx tsx files (unit tests)

fix: unit test code formating changes

fix: unit test code formating changes

fix: unit test code formating changes

fix: unit test code formating changes

fix: lwc compiler errors on jsx tsx files
@SourabhMulay SourabhMulay force-pushed the Saurabh/Fix-compiler-error-on-jsx/tsx-files branch from 84af21a to f95a6ef Compare April 24, 2024 05:03
@SourabhMulay
Copy link
Contributor Author

Yeah, but this is my personal account. I have added the Name as well email in the latest commit. It should have now recognise me as a GitHub user.

The older commits still show the invalid email address. I believe you will need to do an interactive rebase or squash-merge followed by a force-push to fix this.

Hi @nolanlawson ,
I have changed the commit author for all the commits. and also have squashed (2 to N) commits together. Thanks for the suggestion, I have learned a lot.

expect(result.cssScopeTokens).toBeUndefined();
});
}
['.js', '.jsx', '.ts', '.tsx'].forEach(testFileExtensionSupport);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@nolanlawson
Copy link
Contributor

/nucleus test

@nolanlawson nolanlawson merged commit c0a565b into salesforce:master Apr 24, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants