Skip to content
This repository has been archived by the owner on Apr 21, 2020. It is now read-only.

fix(build): prevent scoped variables become global after transpilation #42

Closed
wants to merge 2 commits into from

Conversation

kcmr
Copy link
Contributor

@kcmr kcmr commented Apr 20, 2020

The build process of the polymer-build fork includes a Babel plugin to transform block scopes used at the top level of a file to an IIFE. This prevents side effects caused by exposing variables like Element (native) in window. This global variable exposing happens after transpilation to ES5, that transforms const and let to var, so the block scope has no effect.

Original code:

{
  const { Element } = Polymer;
}

After transpilation:

{
  var Element = _Polymer.Element;
}

Using the included Babel plugin:

(function() {
  'use strict';
  var Element = _Polymer.Element;
}());

The Polymer Tools monorepo, where the original polymer-build package is located, is unmaintained and Polymer has announced that they will no longer contribute to its own tools, so we have created a "fork" in BBVAEngineering org to include this change: https://github.com/BBVAEngineering/polymer-build

The build process of the fork includes a Babel
plugin to transoform top level block scope to IIFE.
This prevents having variables like Element exposed in window (side effects).
@kcmr kcmr requested review from a team April 20, 2020 14:19
package.json Outdated
@@ -32,7 +32,7 @@
"fs-extra": "^8.1.0",
"html-postcss": "^0.1.2",
"merge-stream": "^2.0.0",
"polymer-build": "^3.1.2",
"polymer-build": "git+https://github.com/BBVAEngineering/polymer-build.git#v3.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a published package instead of a repo tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use a published package changing the package name or using a scope...

Copy link
Contributor

@adrigzr adrigzr Apr 20, 2020

Choose a reason for hiding this comment

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

Did you open a PR to the main package in order to try to merge the change? I don't personally like cloning 3th party packages and maintaining them...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try it but it's not going to be merged. There are more than 1000 open issues and the repo is unmaintained. I think that this package (polymer-build) is not going to receive new PRs and wont require maintenance. However, if we publish it with another name, we have to maintain it, set a proper publish / build process, etc.

Another option, not good (very bad in fact) since this package requires modules of polymer-build explicitly, is to include it as peerDependency and let the app specify the version to use.

Third option: include the package as a bundledDependency.

Fourth: move everything (polymer-build, this package and ember-cli-polymer-bundler) to private repos. I think that this option does not deserve the effort for the time we, and maybe other people, are going to use Polymer.

As a last resource, I can publish it in npm from my Github account. I'm not going to maintain the repo, except for security updates, but at least it will be published in npm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR in Polymer/tools: Polymer/tools#3502

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to build-polymer (published in npm) instead of polymer-build.
I don't have permissions to delete the fork of polymer-build in BBVAEngineering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a leave the company and the team needs to make some modification to that package, you'll probably need to make a fork again... That was the point of moving the fork here.

@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #42 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #42   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           64        64           
=========================================
  Hits            64        64           
Impacted Files Coverage Δ
lib/index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a45391b...6d37832. Read the comment docs.

build-polymer is the same package with an additional Babel plugin
@kcmr kcmr closed this Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants