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

feat(compiler): add babel preset to remove type info for TS support #3471

Closed
wants to merge 3 commits into from

Conversation

rui-rayqiu
Copy link
Contributor

Details

Add babel preset @babel/preset-typescript so that when a component is written in TypeScript the compiler can strip off type info.

Also add an example in the playground to show that a component written in TypeScript.

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.

GUS work item

W-13048054

Comment on lines 71 to 74
const { code, map } = transformSync(actual, 'foo.ts', {
...TRANSFORMATION_OPTIONS,
outputConfig: { sourcemap: true },
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This block of code has been repeated in all the test cases, consider moving it into verifyTransformedCode and update that method's name.
Assert that there are no warnings.

Suggested change
const { code, map } = transformSync(actual, 'foo.ts', {
...TRANSFORMATION_OPTIONS,
outputConfig: { sourcemap: true },
});
const { code, map, warnings } = transformSync(actual, 'foo.ts', {
...TRANSFORMATION_OPTIONS,
outputConfig: { sourcemap: true },
});
expect(warnings).toBeUndefined();

@@ -51,3 +51,139 @@ it('should object spread', async () => {
expect(code).toContain('b: 1');
expect(code).not.toContain('...a');
});

describe('should transform TypeScript file correctly', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Add a negative test case to show what happens when a TS failure occurs(I understand that TS validation is not part of the oss compiler. But lets establish that oss compiler still works fine even if there is invalid type specification.
  2. What happens when a component imports a Type Definition? For example import type {SquareConfig} from './types';

verifyTransformedCode(code, map, '<Type>');
});

it('should work for decorators', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good test case.

Copy link
Member

@pmdartus pmdartus left a comment

Choose a reason for hiding this comment

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

Have you considered running TypeScript as part of the lwc-platform compiler?

There are multiple downsides to running type stripping as part of LWC code transformation:

  • slows down all JavaScript transformation which can be problematic for server-side generated components.
  • The way it is currently implemented, it would always attempt to run type stripping even if the file isn't a TypeScript file. This preset should only be enabled when processing .ts file.
  • The LWC OSS compiler will be bound to the certain syntax of TypeScript which isn't great if the LWC compiler version isn't updated as frequently as the TypeScript compiler version in downstream projects.

@rui-rayqiu
Copy link
Contributor Author

Thanks @pmdartus!

  • slows down all JavaScript transformation which can be problematic for server-side generated components.
  • The way it is currently implemented, it would always attempt to run type stripping even if the file isn't a TypeScript file. This preset should only be enabled when processing .ts file.

For the first two points, I think I can try to add Babel TypeScript plugin conditionally so that it is only used when the file extension is .ts and for all .js files the code should remain the same to avoid unnecessary cost.

  • The LWC OSS compiler will be bound to the certain syntax of TypeScript which isn't great if the LWC compiler version isn't updated as frequently as the TypeScript compiler version in downstream projects.

For the third point, it is true that there might be a version mismatch problem for the downstream projects. But do we want to support users of OSS or use cases outside of platform compiler to use TS? If we do want to support, this kind of change seems necessary. Or do you think that there is no need for supporting OSS on TS for some reason? Also would the same problem exist in lwc-platform too?

@rui-rayqiu
Copy link
Contributor Author

rui-rayqiu commented Apr 27, 2023

Update: I've thought about handling TS transformation in lwc-platform, I think it is possible to add a Rollup plugin for Babel that transforms TS into JS and maybe Rollup/Babel will keep the source map correct. But that would leave out OSS compiler and anyone using OSS compiler with Rollup plugin would need to config it themselves for TS such as described in this #2715. If we do this change in OSS compiler then the change is in one place since platform compiler calls OSS compiler.

@pmdartus
Copy link
Member

pmdartus commented May 3, 2023

But do we want to support users of OSS or use cases outside of platform compiler to use TS?

Yes, we should also support TypeScript in OSS. That said, the LWC compiler shouldn't be in charge of transforming the TypeScript source into JavaScript. If you look at how other frameworks offer TypeScript support, the TypeScript source is transformed into valid JavaScript prior to invoking the framework compiler.

For instance, the LWC + TypeScript + Rollup would look like this:

import typescript from '@rollup/plugin-typescript';
import lwc from '@lwc/rollup-plugin';

export detault {
  input: './src/app.ts',
  plugins: [
    typescript(),
    lwc()
  ],
}

If we do this change in OSS compiler then the change is in one place since platform compiler calls OSS compiler.

I agree with your sentiment that a battery-included compiler, with built-in TypeScript support, appears to be ideal from a consumer perspective. But, it's important to acknowledge building a TypeScript file is complex. Even if we come up with sensible defaults, some consumers would always want to customize the build.

For this reason, I would rather recommend keeping the TypeScript transformation step separated from the LWC compilation step. In addition, I would recommend investing in documentation and examples to help developers set up a LWC + TypeScript project.

@nolanlawson
Copy link
Contributor

@pmdartus's comment makes sense to me. Looking at how Svelte does it, they just recommend a vanilla Rollup setup with @rollup/plugin-typescript and typescript. Consumers are responsible for providing those dependencies. Vue appears to do something similar.

@rui-rayqiu
Copy link
Contributor Author

Thanks @pmdartus and @nolanlawson. I think your points make sense about separating the LWC compiler and TS type stripping. I just thought that having a built-in support for TS files inside LWC compiler could ease the process of setting up a TS + LWC project and it still allows customization with Rollup or any other tools to transform TS into JS first if consumer wants. But I agree that we should separate them and provide better documentation and examples.

Should we then keep TypeScript type stripping outside of lwc-platform compilation step too for consistency? I think the argument there is similar. Or is it different for Salesforce internal teams that we have more control on build process so they should not setup their own TS to JS transformation (with good documentation and examples to help them)?

@rui-rayqiu rui-rayqiu closed this May 4, 2023
@rui-rayqiu rui-rayqiu deleted the rui/ts-support branch May 4, 2023 18:06
@rui-rayqiu
Copy link
Contributor Author

Closing this PR as we decided not to add TS related implementation in OSS compiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants