Skip to content

refactor(webpack/Compiler): remove duplicated assets in assetCache#276

Merged
RafikiTiki merged 1 commit into
mainfrom
refactor/remove-dupliated-assets-in-assetsCache
Dec 13, 2022
Merged

refactor(webpack/Compiler): remove duplicated assets in assetCache#276
RafikiTiki merged 1 commit into
mainfrom
refactor/remove-dupliated-assets-in-assetsCache

Conversation

@RafikiTiki
Copy link
Copy Markdown
Collaborator

Summary

This PR slightly refactors what was already implemented by @meypod in #256 – instead of duplicating each asset under a key which is valid pathname for Windows I'm modifying (or not) the filename based on the platform in context of which Complier is run. This should be safe as it will not affect production builds because the Compiler in only used with devServer.

Test plan

Unfortunately I don't have access to a machine running Windows – @meypod would you be able to test that change and let me know if it's working properly on your end?

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Dec 12, 2022

🦋 Changeset detected

Latest commit: e435bbc

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

This PR includes changesets to release 1 package
Name Type
@callstack/repack Minor

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

Copy link
Copy Markdown
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

LGTM


export const adaptFilenameToPlatform = (filename: string) => {
if (isWindows) {
return filename.replace(/\\/g, '/');
Copy link
Copy Markdown
Member

@thymikee thymikee Dec 12, 2022

Choose a reason for hiding this comment

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

not sure if we use it already, but if we do, we could use slash library to do so

@RafikiTiki RafikiTiki force-pushed the refactor/remove-dupliated-assets-in-assetsCache branch from c097271 to e435bbc Compare December 12, 2022 14:58
@meypod
Copy link
Copy Markdown
Contributor

meypod commented Dec 12, 2022

Hi
thanks for this
I'll try to make these changes manually in node_modules and get back to tell you if it works within the hour
but I wonder if it's important to not have duplicated assets on windows ? since my change should be noop for linux users

@meypod
Copy link
Copy Markdown
Contributor

meypod commented Dec 12, 2022

tested, seems to be working as expected, didn't encounter any issue

@RafikiTiki RafikiTiki merged commit a15e881 into main Dec 13, 2022
@RafikiTiki RafikiTiki deleted the refactor/remove-dupliated-assets-in-assetsCache branch December 13, 2022 10:35
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.

3 participants