Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Use webpack native json behavior for loading json files via assetUrl #745

Merged
merged 2 commits into from Mar 21, 2019

Conversation

micburks
Copy link
Contributor

No description provided.

@rtsao
Copy link
Member

rtsao commented Mar 20, 2019

Unless I'm mistaken, I think we should be able to remove json-loader.js entirely since this is built into webpack 4?

Since this is a breaking change, we should figure out how we will want to ship this (potentially codemods?) and if there's any other breaking changes we want to make in v2 before releasing this.

@micburks
Copy link
Contributor Author

@rtsao I thought so too, but removing it entirely doesn't seem to work with json files that were added to the build via assetUrl

@rtsao
Copy link
Member

rtsao commented Mar 20, 2019

Ah, I see, it must be taking precedence over file-loader?

@micburks
Copy link
Contributor Author

The comment in the json-loader also mentioned this would be a breaking change, but I haven't been able to identify how this breaks for a consumer. The only effect should be that unused keys should be removed from the client-main build. Am I missing something?

@rtsao
Copy link
Member

rtsao commented Mar 21, 2019

After comparing webpack v3 and v4 behavior, I think you are right -- there is no breaking change. I recall looking into this earlier and there was some different behavior regarding default exports, but maybe I was mistaken or this has changed in the meantime.

rtsao
rtsao previously approved these changes Mar 21, 2019
@@ -89,11 +89,25 @@ test('`fusion dev` works with assets', async () => {
);
t.equal(await request(`${url}/dirname`), 'src');
t.equal(await request(`${url}/filename`), 'src/main.js');

const jsonAssetUrl = await request(`${url}/json`);
const jsonAsset = (await request(url + jsonAssetUrl)).replace(/\s/g, '');
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: maybe use JSON.parse and t.deepEqual instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better. Thanks!

@micburks
Copy link
Contributor Author

!merge

@old-fusion-bot old-fusion-bot bot merged commit 439cdba into master Mar 21, 2019
@old-fusion-bot
Copy link

Triggered Fusion.js build verification: https://buildkite.com/uberopensource/fusion-release-verification/builds/1770

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants