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.

Sign up for GitHub

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 relative paths in CSS (take 2) #2770

Closed
wants to merge 5 commits into from
Closed

Fix relative paths in CSS (take 2) #2770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b1165bf
Correctly build URLs for CSS
mischnic Mar 7, 2019
4e0024f
Fix tests
mischnic Mar 8, 2019
026a8c9
Return relative path from addURLDependency
devongovett Mar 11, 2019
04de070
Add test for import a url to a raw asset in a subfolder
devongovett Mar 11, 2019
477b1a3
Fix test
devongovett Mar 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions packages/core/integration-tests/test/css.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,21 +272,15 @@ describe('css', function() {
}
]);

let css = await fs.readFile(
path.join(__dirname, '/dist/a/style1.css'),
'utf8'
);
let cssPath = path.join(__dirname, '/dist/a/style1.css');
let css = await fs.readFile(cssPath, 'utf8');

assert(css.includes('background-image'), 'includes `background-image`');
assert(/url\([^)]*\)/.test(css), 'includes url()');

assert(
await fs.exists(
path.join(
__dirname,
path.dirname('/dist/a/style1.css'),
css.match(/url\(([^)]*)\)/)[1]
)
path.join(path.dirname(cssPath), css.match(/url\(([^)]*)\)/)[1])
),
'path specified in url() exists'
);
Expand Down
1 change: 1 addition & 0 deletions packages/core/integration-tests/test/integration/import-raw-subfolder/foo/test.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hi there
5 changes: 5 additions & 0 deletions packages/core/integration-tests/test/integration/import-raw-subfolder/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const url = require('./foo/test.txt');

module.exports = function () {
return url;
};
26 changes: 26 additions & 0 deletions packages/core/integration-tests/test/javascript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,32 @@ describe('javascript', function() {
assert(await fs.exists(path.join(__dirname, '/dist/', output())));
});

it('should support importing a URL to a raw asset in a subfolder', async function() {
let b = await bundle(
path.join(__dirname, '/integration/import-raw-subfolder/index.js')
);

await assertBundleTree(b, {
name: 'index.js',
assets: ['index.js', 'test.txt'],
childBundles: [
{
type: 'map'
},
{
type: 'txt',
assets: ['test.txt'],
childBundles: []
}
]
});

let output = await run(b);
assert.equal(typeof output, 'function');
assert(/^\/test\.[0-9a-f]+\.txt$/.test(output()));
assert(await fs.exists(path.join(__dirname, '/dist/', output())));
});

it('should minify JS in production mode', async function() {
let b = await bundle(path.join(__dirname, '/integration/uglify/index.js'), {
production: true
Expand Down
8 changes: 4 additions & 4 deletions packages/core/parcel-bundler/src/Asset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,12 @@ class Asset {

this.addDependency(depName, Object.assign({dynamic: true, resolved}, opts));

const parsed = URL.parse(url);
parsed.pathname = this.options.parser
let parsed = URL.parse(url);
let name = this.options.parser
.getAsset(resolved, this.options)
.generateBundleName();

parsed.pathname = path.relative(path.dirname(this.relativeName), name);
Copy link
Member

@mischnic mischnic Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

name is always just a hash+extension, so this relative call doesn't really do anything useful.

Here's a repo where it still isn't working (the error resurfaced I tried to fix the original relative PR): https://github.com/mischnic/2736-svg-import
In the dist root, a CSS file contains url('../logo_color.78fe8740.svg');.

(I also think this should be path.posix.relative to not generate backslashes on Windows.)

Copy link
Member Author

Choose a reason for hiding this comment

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

the test for CSS that I updated fails without this line...

Copy link
Member

@mischnic mischnic Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is the core problem in the repo I linked above:

Relative name of asset: a/style.css. A URL Dependency is in the outputDir root, so ../logo_color.78fe8740.svg seems correct.

Actually, a/style.css gets put into the root as well (does the Asset know this or only the Packager?) and now the ../ part is wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

ahhh you're right. I guess we cannot correctly generate the relative path.

return URL.format(parsed);
}

Expand Down Expand Up @@ -257,8 +258,7 @@ class Asset {
// Replace temporary bundle names in the output with the final content-hashed names.
let newValue = value;
for (let [name, map] of bundleNameMap) {
let mapRelative = path.relative(path.dirname(this.relativeName), map);
newValue = newValue.split(name).join(mapRelative);
newValue = newValue.split(name).join(map);
}

// Copy `this.generated` on write so we don't end up writing the final names to the cache.
Expand Down
7 changes: 6 additions & 1 deletion packages/core/parcel-bundler/test/asset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ describe('Asset', () => {
}
}
};
const asset = new Asset('test', options);
const asset = new Asset('/root/dir/test', options);

it('should ignore urls', () => {
const url = 'https://parceljs.org/assets.html';
Expand All @@ -63,6 +63,11 @@ describe('Asset', () => {
assert.strictEqual(asset.addURLDependency('foo'), bundleName);
});

it('should generate relative path', () => {
const asset = new Asset('/root/dir/test/baz', options);
assert.strictEqual(asset.addURLDependency('foo'), '../' + bundleName);
});

it('should preserve query and hash', () => {
assert.strictEqual(
asset.addURLDependency('foo#bar'),
Expand Down