-
Notifications
You must be signed in to change notification settings - Fork 58
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: add Analog framework to framework detection #5751
Conversation
test('should not detect angular', async ({ fs }) => { | ||
const cwd = mockFileSystem({ | ||
'package.json': JSON.stringify({ dependencies: { '@analogjs/platform': '*', '@angular/cli': '*' } }), | ||
'angular.json': '', | ||
}) | ||
const detected = await new Project(fs, cwd).detectFrameworks() | ||
const detectedFrameworks = (detected ?? []).map((framework) => framework.id) | ||
expect(detectedFrameworks).not.toContain('angular') | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does feel weird to test here? angular test also feels weird - if there are better ideas for this kind of test which is dependent on both analog and angular detection - happy to adjust this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope I think as this is a angular meta framework this is exactly the right test I was looking for!.
We want to detect the meta-framework not the framework in this case, and this test verifies that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for fixing!
name = 'Analog' | ||
configFiles = [] | ||
npmDependencies = ['@analogjs/platform'] | ||
category = Category.SSG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The category classification might be a bit odd, but it matches with category we have for Next.js:
category = Category.SSG |
Available categories are
build/packages/build-info/src/frameworks/framework.ts
Lines 8 to 12 in 33dbdb4
export enum Category { | |
FrontendFramework = 'frontend_framework', | |
SSG = 'static_site_generator', | |
BuildTool = 'build_tool', | |
} |
and SSG name is probably misleading as in essence it has both SSGs and overall meta frameworks - so kind of like ~fully featured thing allowing to build a non-SPA sites and I think it's due to history (started with SSG and over time as meta frameworks were being added to same categorization but category name was never updated to reflect that)
🎉 Thanks for submitting a pull request! 🎉
Summary
This adds Analog.js to framework detection.
Note:
Right now Analog.js also does require setting Netlify Functions dir to
dist/analog
so this doesn't fully solve analog, but:.netlify/functions-internal
) and output the function in expected location instead of expecting users (or us) to adjust where we look for functionsI'll try to open github issue or straight fix this in analog for things to just work
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)