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

Fix for #9673 #9680

Merged
merged 4 commits into from
Jan 24, 2024
Merged

Fix for #9673 #9680

merged 4 commits into from
Jan 24, 2024

Conversation

loucyx
Copy link
Contributor

@loucyx loucyx commented Jan 12, 2024

Changes

Closes #9673

  • Added a .js to the end of the configPath for types, making it work for verbatim modules in TypeScript.
  • I also moved the logic to its own function, making it more readable.

Testing

Ran all the tests with no errors reported. I didn't add tests because I didn't think they were needed, but if you think otherwise, I'll need guidance on testing different TSConfigs on a given file.

Docs

Docs don't need to be updated. This is a fix for users with verbatim modules in TypeScript, but it works the same for users without that setting.

Copy link

changeset-bot bot commented Jan 12, 2024

🦋 Changeset detected

Latest commit: 84f8d68

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jan 12, 2024
@florian-lefebvre florian-lefebvre linked an issue Jan 12, 2024 that may be closed by this pull request
1 task
Co-authored-by: Florian Lefebvre <[email protected]>
return JSON.stringify(
`${isRelativePath(normalizedConfigPath) ? '' : './'}${
normalizedConfigPath.endsWith('.ts')
? normalizedConfigPath.replace(/\.ts$/, '')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this always resolves to a file that ends with .ts or .js then we can simplify normalizeConfigPath a bit by moving that .js at the end of the returned string here, so the line ends up being:

? normalizedConfigPath.replace(/\.ts$/, ".js")

Copy link
Contributor

@bholmesdev bholmesdev Jan 24, 2024

Choose a reason for hiding this comment

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

Fair point. I believe .mjs would be possible, and this change would silently leave the .mjs extension without changing it. I'd prefer that to be the case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll update the PR with the simplified code, then. Thank you!

packages/astro/src/content/types-generator.ts Outdated Show resolved Hide resolved

return JSON.stringify(
`${isRelativePath(normalizedConfigPath) ? '' : './'}${
normalizedConfigPath.endsWith('.ts')
Copy link
Member

Choose a reason for hiding this comment

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

There are also .d.ts files. Are they covered by this check on purpose?

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK it should not be .d.ts? So is it worth handling this case?

Copy link
Contributor Author

@loucyx loucyx Jan 18, 2024

Choose a reason for hiding this comment

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

If we never get .d.ts and is always .ts then we can update this to replace .ts with .js simplifying the code a bit:

function normalizeConfigPath(from: string, to: string) {
	const normalizedConfigPath = path.relative(from, to);

	return `"${
		isRelativePath(normalizedConfigPath) ? "" : "./"
	}${normalizedConfigPath.replace(/\.ts$/, ".js")}"`;
}

Copy link
Member

Choose a reason for hiding this comment

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

If we never get .d.ts

That's what I not sure about (I am not familiar with this part of the code). So maybe keeping the code as is better.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this looks like the loader for src/content/config.ts correct? If so, we only support .ts and .js. .d.ts shouldn't be possible 👍

Copy link
Contributor

@bholmesdev bholmesdev left a comment

Choose a reason for hiding this comment

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

Good find! Like leaning on the .js convention for TypeScript. .jsust ship it 👍

@ematipico ematipico merged commit 5d7db1d into withastro:main Jan 24, 2024
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Jan 24, 2024
@loucyx loucyx deleted the verbatim-content-config-fix branch January 24, 2024 16:52
ematipico pushed a commit that referenced this pull request Jan 25, 2024
* Fix for #9673

* 🦋 add changeset file

* Update breezy-plants-smoke.md

Co-authored-by: Florian Lefebvre <[email protected]>

* ⚡️ simplified normalizeConfigPath

---------

Co-authored-by: Florian Lefebvre <[email protected]>
ematipico added a commit that referenced this pull request Jan 31, 2024
* i18n(domains): validation and updated logic (#9099)

* feat(i18n): domain with lookup table (#9112)

* chore: add changelog, fix types and enable experimental support in node/vercel

* rebase and update lock file

* chore: fix failing test

* Apply suggestions from code review

Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by:  Matthew Phillips <[email protected]>

* Update .changeset/tidy-carrots-jump.md

Co-authored-by: Sarah Rainsberger <[email protected]>

* wip

* chore: rebase, conflicts and tests

* update lock file

* chore: correct configuration

* chore: correct configuration

* fix: regressions

* chore: fix conflicts and add more tests

* chore: add more validation

* chore: more tests and add more restrictions

* fix changeset

* change and revert adapters

* add another restriction

* lock file update

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Sarah Rainsberger <[email protected]>

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Bjorn Lu <[email protected]>

* wat

* fix syntax error

* fix config example

* Fix for #9673 (#9680)

* Fix for #9673

* 🦋 add changeset file

* Update breezy-plants-smoke.md

Co-authored-by: Florian Lefebvre <[email protected]>

* ⚡️ simplified normalizeConfigPath

---------

Co-authored-by: Florian Lefebvre <[email protected]>

* Fix env var replacement for export const prerender (#9807)

* feat(alpinejs): allow customizing the Alpine instance (#9751)

* feat(alpinejs): allows customzing the Alpine instance

* chore: add e2e tests

* fix: rename script

* Update index.ts

* fix: lockfile

* [ci] format

* chore: use correct lock file

* chore: rebase

* fix regressions in tests

* fix regressions in tests

* fix build

* add description

* fix missing types

* chore: fix tests, again :D

* eslint

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Nate Moore <[email protected]>

* chore: address feedback

* chore: fix regressions

* chore: refactor naming

* Update packages/astro/src/core/app/index.ts

Co-authored-by: Bjorn Lu <[email protected]>

* chore: address feedback

* update lock file

* chore: infer routing from options, not strategy

* merge from main

* merge from main

* Experimental support in vercel adapter

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Sarah Rainsberger <[email protected]>

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Sarah Rainsberger <[email protected]>

* Update .changeset/tidy-carrots-jump.md

Co-authored-by: Sarah Rainsberger <[email protected]>

* better changesets

* Updates both experimental.i18nDomains and i18ndomains for experimental strategy

* fix link syntax

* consistent tabs/spaces

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Sarah Rainsberger <[email protected]>

* apply suggestion

---------

Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Matthew Phillips <[email protected]>
Co-authored-by: Bjorn Lu <[email protected]>
Co-authored-by: Lou Cyx <[email protected]>
Co-authored-by: Florian Lefebvre <[email protected]>
Co-authored-by: Florian Lefebvre <[email protected]>
Co-authored-by: Nate Moore <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong import path for types.d.ts ContentConfig
4 participants