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

feat(lwc-compiler): add baseDir support #375

Merged
merged 6 commits into from
Jun 6, 2018

Conversation

midzelis
Copy link
Contributor

@midzelis midzelis commented Jun 5, 2018

Used by playground to compile sub components. Need baseDir to
indicate position of root component within files object.

Details

Does this PR introduce a breaking change?

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:
Please check if your PR fulfills the following requirements:

Used by playground to compile sub components. Need baseDir to
indicate position of root component within files object.
@midzelis midzelis requested review from pmdartus and apapko June 5, 2018 15:13
Copy link
Collaborator

@apapko apapko left a comment

Choose a reason for hiding this comment

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

two small change:

  1. missing build:umd script, i believe it was removed from lwc-compiler/packages.json
  2. please add a test to cover baseDir functionality as well as the negative scenario when importer is undefined and baseDir is not specified.

@@ -1,5 +1,9 @@
import { isBoolean, isString, isUndefined } from "../utils";

const DEFAULT_OPTIONS = {
baseDir: "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

it would be great to have a small comment explaining the need for the baseDir

return;
}

const relPath = importee ? path.dirname(importee) : "";
let absPath = path.join(relPath, id);
const relPath = importer ? path.dirname(importer) : options.baseDir || "";
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need a more specific error message when importer is undefined and baseDir is not specified or should the line 65 take care of it?

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 4c0720a | Target commit: 8309707

lwc-engine-benchmark

table-append-1k metric base(4c0720a) target(8309707) trend
benchmark-table/append/1k duration 147.10 (± 4.70 ms) 153.40 (± 7.20 ms) -4.28% 👎
table-clear-1k metric base(4c0720a) target(8309707) trend
benchmark-table/clear/1k duration 11.80 (± 0.50 ms) 11.60 (± 0.60 ms) 1.69% 👍
table-create-10k metric base(4c0720a) target(8309707) trend
benchmark-table/create/10k duration 861.85 (± 7.05 ms) 847.20 (± 5.50 ms) 1.70% 👍
table-create-1k metric base(4c0720a) target(8309707) trend
benchmark-table/create/1k duration 101.20 (± 2.30 ms) 102.25 (± 2.25 ms) -1.04% 👌
table-update-10th-1k metric base(4c0720a) target(8309707) trend
benchmark-table/update-10th/1k duration 92.40 (± 5.20 ms) 92.60 (± 4.80 ms) -0.22% 👌
tablecmp-append-1k metric base(4c0720a) target(8309707) trend
benchmark-table-component/append/1k duration 257.10 (± 4.20 ms) 256.40 (± 4.50 ms) 0.27% 👌
tablecmp-clear-1k metric base(4c0720a) target(8309707) trend
benchmark-table/clear/1k duration 36.00 (± 2.30 ms) 36.00 (± 1.90 ms) 0.00% 👌
tablecmp-create-10k metric base(4c0720a) target(8309707) trend
benchmark-table-component/create/10k duration 1675.10 (± 8.20 ms) 1700.50 (± 8.60 ms) -1.52% 👎
tablecmp-create-1k metric base(4c0720a) target(8309707) trend
benchmark-table-component/create/1k duration 199.30 (± 5.00 ms) 199.20 (± 4.00 ms) 0.05% 👌
tablecmp-update-10th-1k metric base(4c0720a) target(8309707) trend
benchmark-table-component/update-10th/1k duration 74.50 (± 4.70 ms) 75.40 (± 5.00 ms) -1.21% 👌

Copy link
Collaborator

@apapko apapko left a comment

Choose a reason for hiding this comment

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

One small change for the jest test

...COMPILER_CONFIG_BASEDIR
};
delete config.baseDir;
try {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it is better to use expect().rejects to ensure rejection is triggered

await expect(compile(config)).rejects.toMatchObject({
            message: Could not resolve 'foo' (as foo.js) from compiler entry point
        });
    });

if (importer) {
throw new Error(`Could not resolve '${importee}' (as ${absPath}) from '${importer}'`);
}
throw new Error(`Could not resolve '${importee}' (as ${absPath}) from compiler entry point`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for the explicit error!

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: a4c21a0 | Target commit: 36aaf3d

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: a4c21a0 | Target commit: 2673ab9

lwc-engine-benchmark

table-append-1k metric base(a4c21a0) target(2673ab9) trend
benchmark-table/append/1k duration 149.10 (± 3.90 ms) 144.80 (± 3.30 ms) 2.88% 👍
table-clear-1k metric base(a4c21a0) target(2673ab9) trend
benchmark-table/clear/1k duration 11.70 (± 0.40 ms) 11.50 (± 0.40 ms) 1.71% 👌
table-create-10k metric base(a4c21a0) target(2673ab9) trend
benchmark-table/create/10k duration 856.50 (± 3.60 ms) 845.20 (± 7.60 ms) 1.32% 👍
table-create-1k metric base(a4c21a0) target(2673ab9) trend
benchmark-table/create/1k duration 103.90 (± 1.20 ms) 103.30 (± 2.60 ms) 0.58% 👌
table-update-10th-1k metric base(a4c21a0) target(2673ab9) trend
benchmark-table/update-10th/1k duration 92.70 (± 4.90 ms) 91.50 (± 5.00 ms) 1.29% 👌
tablecmp-append-1k metric base(a4c21a0) target(2673ab9) trend
benchmark-table-component/append/1k duration 258.60 (± 4.00 ms) 257.30 (± 3.70 ms) 0.50% 👌
tablecmp-clear-1k metric base(a4c21a0) target(2673ab9) trend
benchmark-table/clear/1k duration 35.90 (± 1.70 ms) 35.00 (± 2.00 ms) 2.51% 👌
tablecmp-create-10k metric base(a4c21a0) target(2673ab9) trend
benchmark-table-component/create/10k duration 1688.30 (± 8.20 ms) 1743.00 (± 10.00 ms) -3.24% 👎
tablecmp-create-1k metric base(a4c21a0) target(2673ab9) trend
benchmark-table-component/create/1k duration 198.10 (± 3.50 ms) 195.80 (± 3.65 ms) 1.16% 👌
tablecmp-update-10th-1k metric base(a4c21a0) target(2673ab9) trend
benchmark-table-component/update-10th/1k duration 76.70 (± 4.75 ms) 75.10 (± 5.30 ms) 2.09% 👌

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 905dd78 | Target commit: 2ccdc06

lwc-engine-benchmark

table-append-1k metric base(905dd78) target(2ccdc06) trend
benchmark-table/append/1k duration 145.00 (± 4.40 ms) 144.80 (± 3.50 ms) 0.14% 👌
table-clear-1k metric base(905dd78) target(2ccdc06) trend
benchmark-table/clear/1k duration 11.20 (± 0.40 ms) 11.60 (± 0.30 ms) -3.57% 👎
table-create-10k metric base(905dd78) target(2ccdc06) trend
benchmark-table/create/10k duration 845.10 (± 5.10 ms) 846.60 (± 4.60 ms) -0.18% 👌
table-create-1k metric base(905dd78) target(2ccdc06) trend
benchmark-table/create/1k duration 102.10 (± 1.90 ms) 100.20 (± 2.10 ms) 1.86% 👍
table-update-10th-1k metric base(905dd78) target(2ccdc06) trend
benchmark-table/update-10th/1k duration 92.80 (± 4.50 ms) 88.70 (± 5.60 ms) 4.42% 👌
tablecmp-append-1k metric base(905dd78) target(2ccdc06) trend
benchmark-table-component/append/1k duration 259.10 (± 5.20 ms) 254.20 (± 3.80 ms) 1.89% 👍
tablecmp-clear-1k metric base(905dd78) target(2ccdc06) trend
benchmark-table/clear/1k duration 34.70 (± 1.30 ms) 35.40 (± 1.80 ms) -2.02% 👌
tablecmp-create-10k metric base(905dd78) target(2ccdc06) trend
benchmark-table-component/create/10k duration 1684.40 (± 8.40 ms) 1716.20 (± 7.60 ms) -1.89% 👎
tablecmp-create-1k metric base(905dd78) target(2ccdc06) trend
benchmark-table-component/create/1k duration 193.70 (± 3.70 ms) 197.30 (± 4.50 ms) -1.86% 👎
tablecmp-update-10th-1k metric base(905dd78) target(2ccdc06) trend
benchmark-table-component/update-10th/1k duration 74.60 (± 3.90 ms) 75.40 (± 6.00 ms) -1.07% 👌

@apapko apapko merged commit d1cf4a7 into master Jun 6, 2018
@apapko apapko deleted the midzelis/compiler-playground-fix branch June 6, 2018 22:12
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.

2 participants