fix: simplify rollup package #659
Conversation
|
@dbajric This will probably break the current best configuration for IE11. |
| }, | ||
| "dependencies": { | ||
| "es5-proxy-compat": "0.19.0", | ||
| "es5-proxy-compat": "0.20.3", |
There was a problem hiding this comment.
Shouldn't es5-proxy-compat be a devDependency now?
| "eslint": "~4.13.0", | ||
| "eslint-plugin-lwc": "^0.2.4", | ||
| "express": "^4.15.2", | ||
| "lwc-compiler": "0.29.1", | ||
| "lwc-engine": "0.29.1", | ||
| "lwc-jest-transformer": "0.17.15", | ||
| "rollup": "0.60.1", | ||
| "rollup-plugin-compat": "0.19.0", | ||
| "rollup-plugin-compat": "0.20.3", |
There was a problem hiding this comment.
If all the compat related packages are now devDependencies can we hoist them at the root of the monorepo? This would avoid running into version discrepancies.
| @@ -11,7 +11,7 @@ const entry = path.resolve(__dirname, '../../src/framework/main.ts'); | |||
| const commonJSDirectory = path.resolve(__dirname, '../../dist/commonjs'); | |||
| const modulesDirectory = path.resolve(__dirname, '../../dist/modules'); | |||
|
|
|||
| const banner = (`/* proxy-compat-disable */`); | |||
| const banner = (`/* es5-compat-disable */\n/* proxy-compat-disable */`); | |||
There was a problem hiding this comment.
Do we need both pragmas? One should be deprecated now.
There was a problem hiding this comment.
No, we need two because we need to be able to disable in engine any transform at all. That is the only only way to do it in way that doesn't require special one-offs in the rollup plugins
There was a problem hiding this comment.
What is the difference between both pragma?
packages/lwc-compiler/package.json
Outdated
| @@ -19,7 +19,7 @@ | |||
| "@babel/plugin-proposal-object-rest-spread": "7.0.0", | |||
| "@types/chokidar": "^1.7.5", | |||
| "babel-plugin-transform-lwc-class": "0.29.1", | |||
| "babel-preset-compat": "0.19.0", | |||
| "babel-preset-compat": "0.20.3", | |||
There was a problem hiding this comment.
Interesting that the compiler depends on the babel-preset-compat. I am wondering if we can do something here as well to get rid of the direct dependency.
There was a problem hiding this comment.
Why? You would have to depend on all the babel dependencies and the do pretty much the same thing the package is doing. Since we own that package not sure why would we gain.
There was a problem hiding this comment.
I was just wondering. I agree with you.
|
|
||
| it(`simple app`, async () => { | ||
| const lwcPath = getLwcEnginePath(rollupOptions.mode); | ||
| const lwcPath = getLwcEnginePath('compat'); |
There was a problem hiding this comment.
I don't understand exactly what does this tests do, the title is probably wrong since you are not asserting against the generated code.
| @@ -6,9 +6,11 @@ import { defineProperty } from "./language"; | |||
| * creation of symbols, so we can fallback to string when native symbols | |||
| * are not supported. | |||
| */ | |||
| const supportSymbols = Symbol('x').toString() === 'Symbol(x)'; | |||
There was a problem hiding this comment.
This should actually be faster
| @@ -6,9 +6,11 @@ import { defineProperty } from "./language"; | |||
| * creation of symbols, so we can fallback to string when native symbols | |||
| * are not supported. | |||
| */ | |||
| const hasNativeSymbolsSupport = Symbol('x').toString() === 'Symbol(x)'; | |||
There was a problem hiding this comment.
I don't fully understand this change. In theory, if the env is patched, this should return true. How is that typeof Symbol() === 'symbol' is not reliable?
There was a problem hiding this comment.
the only case in which the original condition is true: typeof Symbol() === 'symbol' in IE11, is if the polyfill runs first, and theengine itself gets transformed for typeof
There was a problem hiding this comment.
The polyfill MUST always run first.
Precisely the typeof doesn't work because its translated to _typeof
There was a problem hiding this comment.
for posterity, any symbol polyfill uses the toString() to generate a unique key, while the native has a very well defined output.
additionally, the original condition doesn't work because in compat mode for IE11, that typeof is also transformed, which means that the condition is always true after the polyfill and the transformation occur,
| @@ -6,9 +6,11 @@ import { defineProperty } from "./language"; | |||
| * creation of symbols, so we can fallback to string when native symbols | |||
| * are not supported. | |||
| */ | |||
| const hasNativeSymbolsSupport = Symbol('x').toString() === 'Symbol(x)'; | |||
There was a problem hiding this comment.
the only case in which the original condition is true: typeof Symbol() === 'symbol' in IE11, is if the polyfill runs first, and theengine itself gets transformed for typeof
Benchmark resultsBase commit: lwc-engine-benchmark-ie11
lwc-engine-benchmark
|
Details
Decouple all rollup plugins, simplify code around it.