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.

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

Jump to bottom

Make macro configs static #1815

Merged
merged 2 commits into from
Feb 27, 2024
Merged

Make macro configs static #1815

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
183dd97
Make macro configs (except fastboot) be considered not having a runti…
simonihmig Feb 23, 2024
e55b055
Treat getGlobalConfig().fastboot as dynamic as well
simonihmig Feb 27, 2024
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
47 changes: 44 additions & 3 deletions packages/macros/src/babel/evaluate-json.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type State from './state';
import dependencySatisfies from './dependency-satisfies';
import moduleExists from './module-exists';
import getConfig from './get-config';
import assertNever from 'assert-never';

type OpValue = string | boolean | number;

Expand Down Expand Up @@ -388,13 +389,53 @@ export class Evaluator {
return { confident: true, value: moduleExists(path, this.state), hasRuntimeImplementation: false };
}
if (callee.referencesImport('@embroider/macros', 'getConfig')) {
return { confident: true, value: getConfig(path, this.state, 'package'), hasRuntimeImplementation: true };
return { confident: true, value: getConfig(path, this.state, 'package'), hasRuntimeImplementation: false };
}
if (callee.referencesImport('@embroider/macros', 'getOwnConfig')) {
return { confident: true, value: getConfig(path, this.state, 'own'), hasRuntimeImplementation: true };
return { confident: true, value: getConfig(path, this.state, 'own'), hasRuntimeImplementation: false };
}
if (callee.referencesImport('@embroider/macros', 'getGlobalConfig')) {
return { confident: true, value: getConfig(path, this.state, 'getGlobalConfig'), hasRuntimeImplementation: true };
// Check for getGlobalConfig().fastboot.isRunning, which is the only pattern in use where config actually needs to have a runtime implementation.
// For compatibility reasons we will continue to support that. All other cases of macro configs are static now.
let maybeFastbootMemberExpression = path.parentPath;
if (
maybeFastbootMemberExpression.isMemberExpression() ||
maybeFastbootMemberExpression.isOptionalMemberExpression()
) {
let maybeFastbootProperty = maybeFastbootMemberExpression.isMemberExpression()
? maybeFastbootMemberExpression.get('property')
: maybeFastbootMemberExpression.isOptionalMemberExpression()
? maybeFastbootMemberExpression.get('property')
: assertNever(maybeFastbootMemberExpression);
Copy link
Collaborator Author

@simonihmig simonihmig Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This looks weird, but I found no better way to make TS happy. I had it first as simple as this:

image

You see the return type of .get('property') is not correct, it cannot be an array of NodePath, that makes no sense...

Those tertiary expressions narrow down the type before hitting .get() to make it return the correct type.

The type definition of .get() would not deal well with a union type (NodePath<MemberExpression> | NodePath<OptionalMemeberExpression> it seems, falling back to the second loosely typed one:

image

That tertiary expression would narrow down the type before hitting .get(), returning the expected correct type.


if (maybeFastbootProperty.isIdentifier() && maybeFastbootProperty.node.name === 'fastboot') {
let maybeIsRunningMemberExpression = maybeFastbootMemberExpression.parentPath;
if (
maybeIsRunningMemberExpression.isMemberExpression() ||
maybeIsRunningMemberExpression.isOptionalMemberExpression()
) {
let maybeIsRunningProperty = maybeIsRunningMemberExpression.isMemberExpression()
? maybeIsRunningMemberExpression.get('property')
: maybeIsRunningMemberExpression.isOptionalMemberExpression()
? maybeIsRunningMemberExpression.get('property')
: assertNever(maybeIsRunningMemberExpression);

if (maybeIsRunningProperty.isIdentifier() && maybeIsRunningProperty.node.name === 'isRunning') {
return {
confident: true,
value: getConfig(path, this.state, 'getGlobalConfig'),
hasRuntimeImplementation: true,
};
}
}
}
}

return {
confident: true,
value: getConfig(path, this.state, 'getGlobalConfig'),
hasRuntimeImplementation: false,
};
}
if (callee.referencesImport('@embroider/macros', 'isDevelopingApp')) {
return {
Expand Down
40 changes: 38 additions & 2 deletions packages/macros/tests/babel/eval.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,21 +164,57 @@ describe('hasRuntimeImplementation', function () {
let code = transform(`
import { getConfig } from '@embroider/macros';
const result = getConfig('foo')`);
expect(code).toMatch(`result = true`);
expect(code).toMatch(`result = false`);
});

test('getConfig property access', () => {
let code = transform(`
import { getConfig } from '@embroider/macros';
const result = getConfig('foo').bar`);
expect(code).toMatch(`result = false`);
});

// this is throwing internally, not sure how to fix
test.skip('getOwnConfig', () => {
let code = transform(`
import { getOwnConfig } from '@embroider/macros';
const result = getOwnConfig()`);
expect(code).toMatch(`result = true`);
expect(code).toMatch(`result = false`);
});

test('getGlobalConfig', () => {
let code = transform(`
import { getGlobalConfig } from '@embroider/macros';
const result = getGlobalConfig()`);
expect(code).toMatch(`result = false`);
});

test('getGlobalConfig property access', () => {
let code = transform(`
import { getGlobalConfig } from '@embroider/macros';
const result = getGlobalConfig().foo`);
expect(code).toMatch(`result = false`);
});

test('getGlobalConfig fastboot access', () => {
let code = transform(`
import { getGlobalConfig } from '@embroider/macros';
const result = getGlobalConfig().fastboot`);
expect(code).toMatch(`result = false`);
});

// fastboot.isRunning relies on dynamic evaluation at runtime. For backwards compatibility we keep it working. See https://github.com/embroider-build/embroider/issues/1804
test('getGlobalConfig fastboot.isRunning access', () => {
let code = transform(`
import { getGlobalConfig } from '@embroider/macros';
const result = getGlobalConfig().fastboot.isRunning`);
expect(code).toMatch(`result = true`);
});

test('getGlobalConfig fastboot?.isRunning access', () => {
let code = transform(`
import { getGlobalConfig } from '@embroider/macros';
const result = getGlobalConfig().fastboot?.isRunning`);
expect(code).toMatch(`result = true`);
});

Expand Down
15 changes: 1 addition & 14 deletions packages/macros/tests/babel/macro-condition.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ describe('macroCondition', function () {
expect(code).not.toMatch(/beta/);
});

buildTimeTest('can see booleans inside getConfig', () => {
test('can see booleans inside getConfig', () => {
let code = transform(`
import { macroCondition, getConfig } from '@embroider/macros';
export default function() {
Expand All @@ -373,19 +373,6 @@ describe('macroCondition', function () {
expect(code).not.toMatch(/alpha/);
});

runTimeTest('can see booleans inside getConfig', () => {
let code = transform(`
import { macroCondition, getConfig } from '@embroider/macros';
export default function() {
// this deliberately chains three kinds of property access syntax: by
// identifier, by numeric index, and by string literal.
return macroCondition(getConfig('qunit').items[0]["other"]) ? 'alpha' : 'beta';
}
`);
expect(run(code, { filename })).toBe('beta');
expect(code).toMatch(/alpha/);
});

if (transform.babelMajorVersion === 7) {
buildTimeTest('can be used as class field initializer', () => {
let code = transform(`
Expand Down
Loading