-
Notifications
You must be signed in to change notification settings - Fork 66
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
Refactor file name options #696
Conversation
🦋 Changeset detectedLatest commit: cde40dc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
escapedPatharg := strings.ReplaceAll(patharg, "'", "\\'") | ||
patharg = fmt.Sprintf("\"%s\"", escapedPatharg) |
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 is needed to get test pass as we now test filename
in the tests.
patharg := "import.meta.url" | ||
if tt.filename != "" { | ||
escapedFilename := strings.ReplaceAll(tt.filename, "'", "\\'") | ||
patharg = fmt.Sprintf("\"%s\"", escapedFilename) | ||
} | ||
toMatch += "\n\n" + fmt.Sprintf("export const %s = %s(%s, %s);\n\n", METADATA, CREATE_METADATA, patharg, metadata) |
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 takes care of the filename
value too, instead of import.meta.url
internal/printer/printer_test.go
Outdated
if len(tt.filename) > 0 { | ||
escapedFilename := strings.ReplaceAll(tt.filename, "'", "\\'") | ||
toMatch += suffixWithFilename(escapedFilename) | ||
toMatch = strings.Replace(toMatch, "$$Component", getComponentName((tt.filename)), -1) |
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.
If filename
is passed, the generated code uses a different component name based on it. Replace here too.
test('hash is stable when styles change', () => { | ||
const [a, b] = scopes; | ||
assert.equal(a, b, 'Expected scopes to be equal'); | ||
}); |
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 was removed as explained in the PR description.
Regarding the hash, the biggest thing here is that |
Yup, "normalized" should also mean "root-relative" so the hash should be equal on different machines. To also add to the data point, vite-plugin-svelte does it too (though only for dev it can expand to build too). |
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 looks great! Much easier to reason about.
Changes
sourcefile
andmoduleId
options as a singlefilename
option. (Possible since Simplify HMR handling astro#5811)normalizedFilename
option to generate stable hashes instead.The new scope hashes are derived like this:
normalizedFilename
is define, use it only to generate the hash.I don't think we need to combined both the file path and source to generate the hash, the
normalizedFilename
itself should be unique to generate off if it's define, which Astro core will always pass. The value would be like/src/pages/index.astro
etc.Should also pave the way for withastro/astro#5136 once we are able to use the
normalizedFilename
option in core.Testing
printer_test
to also handle case wherefilename
option is passed and used in the generated code.normalizedFilename
is passed (which we will), the hash will derive from it instead.Docs
n/a