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

Resolve config from extends package #781

Open
wants to merge 3 commits into
base: v1.x
Choose a base branch
from

Conversation

iarroyo
Copy link

@iarroyo iarroyo commented Dec 18, 2024

Resolve glint configuration when your tsconfig.json extends from a package, ex

path: tsconfig.json

{
  "extends": "@package1"
}

path: node_modules/@package1/tsconfig.json

{
  "glint": {
    "environment": ["ember-loose", "ember-template-imports"]
  }
}

Note: I'm uncertain if this is the correct branch to merge with. If it isn't, please let me know.

import SilentError from 'silent-error';
import { GlintConfig } from './config.js';
import { GlintConfigInput } from '@glint/core/config-types';
import type * as TS from 'typescript';

const require = createRequire(import.meta.url);
export const require = createRequire(import.meta.url);
Copy link
Contributor

Choose a reason for hiding this comment

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

why export this?
conventionally, we really only want to use createRequire in the file it's used in, since the resolution rules are specific to the import.meta.url

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, it's for mocking in tests.

can you add a block-comment with something like:

/**
 * @private 
 *
 * Only exported for testing purposes. Do not import.
 */

Copy link
Author

Choose a reason for hiding this comment

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

Done

JSON.stringify({
extends: '@package1/tsconfig.json',
glint: {
environment: '../local-env',
Copy link
Contributor

Choose a reason for hiding this comment

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

what's local-env?

Copy link
Author

Choose a reason for hiding this comment

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

To be honest, I simply followed the example from the previous test which uses a predefined environment in the beforeEach

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

This is looking good -- left some questions.
Also, would you be willing to PR this to the main branch as well?

@iarroyo
Copy link
Author

iarroyo commented Dec 28, 2024

This is looking good -- left some questions. Also, would you be willing to PR this to the main branch as well?

Definitely, I will create a PR against the main branch

@iarroyo
Copy link
Author

iarroyo commented Dec 28, 2024

PR Created
#782

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.

2 participants