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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 3 additions & 9 deletions packages/core/integration-tests/test/css.js
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
@@ -0,0 +1 @@
hi there
@@ -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
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
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

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

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
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