Skip to content

Make filename relative #131

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

Merged
merged 2 commits into from
Oct 19, 2020
Merged

Make filename relative #131

merged 2 commits into from
Oct 19, 2020

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Sep 12, 2020

My main motivation for this PR is that sveltejs/svelte#4856 would embed the filename in the compiled code. I don't think that should be an absolute path because that makes it so that builds are not reproducible (e.g. compiled file will be different if you build in different directory or different machine), can leak information (e.g. my username is in my path), etc.

The filename is used in three existing places today:

  • debug - I think relative path is easier to read for debugging and drops the redundant information
  • sourcemap - the examples I see online use relative paths (e.g. bugsnag article/tutorial), so it seems like an improvement there
  • passed to preprocess - I did an npm link of this PR against svelte-preprocess and all tests passed.

Copy link

@AlbertMarashi AlbertMarashi left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@antony antony added this to the 6.1.0 milestone Oct 14, 2020
@lukeed lukeed modified the milestones: 6.1.0, 7.0.0 Oct 19, 2020
@lukeed
Copy link
Member

lukeed commented Oct 19, 2020

Does this require sveltejs/svelte#5476 parity?

@benmccann
Copy link
Member Author

No. Other way around. I didn't want to merge that PR until we had this change

Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

I checked this out locally & applied it to a project using PostCSS and TypeScript preprocessing. Everything seems to check out well.

Could not validate the claims made by the link svelte PR, but only because my test app is considerably smaller.

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.

4 participants