Skip to content

[frameworks][examples] Fix vite ionic-react framework detection #12880

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.

Sign up for GitHub

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

Merged
onsclom merged 8 commits into from
Jan 22, 2025
Merged

[frameworks][examples] Fix vite ionic-react framework detection #12880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c080fd9
Fix vite ionic-react detection
onsclom Jan 17, 2025
5f759b3
Skip deleted examples
onsclom Jan 21, 2025
b9f7dd6
Add changeset
onsclom Jan 21, 2025
57e4845
Merge branch 'main' into austinmerrick/fix-vite-ionic-react-detection
EndangeredMassa Jan 21, 2025
27282c3
Add test for supersede vite
onsclom Jan 22, 2025
39559d6
Temporarily skip ionic-react example
onsclom Jan 22, 2025
92e3a3f
Merge branch 'austinmerrick/fix-vite-ionic-react-detection' of https:…
onsclom Jan 22, 2025
9adf5f1
Temporarily skip other ionic-react deploy test
onsclom Jan 22, 2025
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
5 changes: 5 additions & 0 deletions .changeset/strange-goats-hang.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@vercel/frameworks': minor
---

Make vite detection supersede ionic-react
3 changes: 2 additions & 1 deletion examples/__tests__/integration/ionic-react.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { deployExample } from '../test-utils';
it('[examples] should deploy ionic-react', async () => {
// TODO: unskip once example is manually changed to `vite`
it.skip('[examples] should deploy ionic-react', async () => {
Comment on lines +2 to +3
Copy link
Copy Markdown
Contributor Author

@onsclom onsclom Jan 22, 2025

Choose a reason for hiding this comment

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

Same as https://github.com/vercel/vercel/pull/12880/files#r1925735759

await deployExample('ionic-react');
});
3 changes: 0 additions & 3 deletions examples/ionic-react/vite.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,5 @@ export default defineConfig({
globals: true,
environment: 'jsdom',
setupFiles: './src/setupTests.ts',
},
build: {
outDir: 'build'
}
})
1 change: 1 addition & 0 deletions packages/frameworks/src/frameworks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1820,6 +1820,7 @@ export const frameworks = [
'Vite is a new breed of frontend build tool that significantly improves the frontend development experience.',
description: 'A Vue.js app, created with Vite.',
website: 'https://vitejs.dev',
supersedes: ['ionic-react'],
Copy link
Copy Markdown
Contributor

@EndangeredMassa EndangeredMassa Jan 21, 2025

Choose a reason for hiding this comment

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

suggestion (blocking): ‏Does this fail because the output dir is dist instead of build? Can we add a test that this behavior is correct?

Copy link
Copy Markdown
Contributor Author

@onsclom onsclom Jan 21, 2025

Choose a reason for hiding this comment

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

Newly initiated ionic-react do not currently deploy with zero config, and this fixes them. These newly inited projects use vite and therefore DO use dist instead of build.

The currently existing e2e tests run the fresh inited ionic react project with vite settings and it succeeds https://github.com/vercel/vercel/actions/runs/12892527405/job/35948586479?pr=12880#step:8:128 (deployment). Is that test sufficient or were you thinking about a different kind of test?

Copy link
Copy Markdown
Contributor

@EndangeredMassa EndangeredMassa Jan 22, 2025

Choose a reason for hiding this comment

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

Ah, thanks for that clarification.

That test was passing before this change. I was hoping to see a test that would only pass because of this change.

What are these more recent build failures on that project?

Copy link
Copy Markdown
Contributor Author

@onsclom onsclom Jan 22, 2025

Choose a reason for hiding this comment

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

Whoops, my bad. Part of making this PR included switching that example to vite manually (since we don't rerun framework detection automatically), but that makes it fail for other PRs if we don't immediately merge this.

Switched it back to ionic react so other PRs will succeed now. Will add a test that shows supersedes prioritizes vite over ionic-react later.

Copy link
Copy Markdown
Contributor Author

@onsclom onsclom Jan 22, 2025

Choose a reason for hiding this comment

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

Added that test now https://github.com/vercel/vercel/pull/12880/files#diff-b9b88879323adf73e47827b03f1d5593292326b4647e78b71f85951f2a3172f8!

envPrefix: 'VITE_',
detectors: {
every: [
Expand Down
8 changes: 7 additions & 1 deletion packages/frameworks/test/frameworks.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,13 @@ async function getDeployment(host: string) {
}

describe('frameworks', () => {
const skipExamples = ['sanity-v3', 'solidstart', 'dojo', 'scully'];
const skipExamples = [
'sanity-v3',
'solidstart',
'dojo',
'scully',
'ionic-react',
];
Comment on lines +218 to +224
Copy link
Copy Markdown
Contributor Author

@onsclom onsclom Jan 22, 2025

Choose a reason for hiding this comment

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

Temporarily adding ionic-react to skipped examples to avoid ionic-react failures.

After this gets merged I will manually set the ionic-react project to vite, then I'll make another PR to unskip the example.


it('ensure there is an example for every framework', async () => {
const root = join(__dirname, '..', '..', '..');
Expand Down
6 changes: 6 additions & 0 deletions packages/fs-detectors/test/unit.framework-detector.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,12 @@ describe('detectFramework()', () => {

expect(await detectFramework({ fs, frameworkList })).toBe('remix');
});

it('Should detect Vite + Ionic React as `vite`', async () => {
const fs = new LocalFileSystemDetector(join(EXAMPLES_DIR, 'ionic-react'));

expect(await detectFramework({ fs, frameworkList })).toBe('vite');
});
});

describe('detectFrameworks()', () => {
Expand Down