Skip to content

[vite] use transform instead of load for gjs & hbs files#1680

Merged
mansona merged 2 commits into
embroider-build:mainfrom
patricklx:patch-3
Dec 13, 2023
Merged

[vite] use transform instead of load for gjs & hbs files#1680
mansona merged 2 commits into
embroider-build:mainfrom
patricklx:patch-3

Conversation

@patricklx
Copy link
Copy Markdown
Contributor

@patricklx patricklx commented Nov 27, 2023

use transform function, this simplifies loading of those files without the need to handle paths

@patricklx patricklx changed the title [vite] allow loading & transforming gts files [vite]use transform instead of load for gts & hbs files Dec 12, 2023
@patricklx patricklx changed the title [vite]use transform instead of load for gts & hbs files [vite] use transform instead of load for gts & hbs files Dec 12, 2023
@patricklx
Copy link
Copy Markdown
Contributor Author

@mansona @ef4

Copy link
Copy Markdown
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

I think overall this is a good PR, it also potentially allows us to tap into some caching of modules because of the caching behaviour of rollup

If the Rollup cache is used (e.g. in watch mode or explicitly via the JavaScript API), Rollup will skip the transform hook of a module if after the load hook, the loaded code is identical to the code of the cached copy.

However I'm not too comfortable expanding the gjs filter to gts because I can imagine it would have a whole list of unintended consequences down the line 🙈 I'm sure it's probably fine but I would prefer that to be in a different PR if that's ok?

Comment thread packages/vite/src/template-tag.ts Outdated
import { Preprocessor } from 'content-tag';

const gjsFilter = createFilter('**/*.gjs?(\\?)*');
const gjsFilter = createFilter('**/*.{gjs,gts}?(\\?)*');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not comfortable adding gts to this for now. I know we should deal with the Typescript situation but I feel like that should be a wholistic PR that explicitly adds support rather than changing this one place

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What is 'the typescript situation'?

For running ts code, babel (or whatever) removes the type annotations and that's it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes but my understanding is that at this level we should always only be seeing gjs files no? and by "situation" I mean fully end-to-end tests on it with typechecking, that should be a different PR in my opinion

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, as of now it would not make any difference. Since currently it's expected that the previous stage compiles the gts files.
It would work together with #1679 . Which would work for vite and then i could make it work in the tests in #1718

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why wouldn't we see the input file ext, if gjs is here already?

If it were compiled, we'd just have js, yeah?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Potentially could also work with webpack if content-tag is added as a loader. Or is there already one?
Why would it work with gjs but not gts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also, .ts is already part of the entry points and vite can load it directly. E.g the vite-app is not setup to transform ts files for the previous stage.
But sure, i can move this gts part elsewhere.
Shall i add it to #1679 ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why wouldn't we see the input file ext, if gjs is here already?

If it were compiled, we'd just have js, yeah?

True, somehow i was thinking gts transforms to gjs first... But it would be js directly, right?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Right, there is no gts to gjs conversion.
I think it goes: gts->ts->js

For content-tag -> babel/whatever

@patricklx patricklx changed the title [vite] use transform instead of load for gts & hbs files [vite] use transform instead of load for gjs & hbs files Dec 13, 2023
@patricklx
Copy link
Copy Markdown
Contributor Author

@mansona i reverted the gts part

Copy link
Copy Markdown
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

great work 🎉

@mansona mansona merged commit 5010f91 into embroider-build:main Dec 13, 2023
@mansona mansona added the enhancement New feature or request label Dec 13, 2023
@github-actions github-actions Bot mentioned this pull request Dec 14, 2023
@github-actions github-actions Bot mentioned this pull request Apr 4, 2024
@github-actions github-actions Bot mentioned this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants