Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

Updates for Polymer 3 #1096

Merged
merged 24 commits into from
Apr 21, 2018
Merged

Updates for Polymer 3 #1096

merged 24 commits into from
Apr 21, 2018

Conversation

bicknellr
Copy link
Member

@bicknellr bicknellr commented Apr 18, 2018

  • Paths are now name specifiers.
  • Removes bower.json.
  • License updates which need to be confirmed.
    • I pulled LICENSE from one of the tools repos because this removes bower.json, which previously linked to the license text.
    • Confirm license text.
  • Fix polymer test; possibly related to polymer serve starting in 'component' mode?
  • Confirm URL format for script tags in HTML. (./node_modules/dep vs ../dep)

@bicknellr
Copy link
Member Author

polymer test doesn't seem to work. Also, there are a couple of extra specifiers I forgot.

Copy link
Contributor

@abdonrd abdonrd left a comment

Choose a reason for hiding this comment

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

I just added changes at #1097.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot
Copy link

☹️ Sorry, but only Googlers may change the label cla: yes.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@bicknellr bicknellr changed the title Add LICENSE file; use name specifiers; remove bower.json Updates for Polymer 3 Apr 18, 2018
LICENSE Outdated
SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this file - canonical license file is already referenced at the top of source files.

Copy link
Contributor

Choose a reason for hiding this comment

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

This files allow us to have the GitHub license preview:

repo-license-indicator

Reference: GitHub Help: Adding a license to a repository

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack on the GitHub license preview, but this is not something we're broadly adopting across our repos at this time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we leaving the license field in package.json?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's fine (needed to suppress npm warning).

polymer.json Outdated
@@ -2,7 +2,8 @@
"entrypoint": "index.html",
"shell": "src/my-app.js",
"sources": [
"images/**/*"
"images/**/*",
"src/**/*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? I've been removing them in other 3.0 projects since it didn't seem to do anything (build discovers the tree through import statements).

Copy link
Contributor

Choose a reason for hiding this comment

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

The truth is that I'm not sure.

Ping to @aomarks @justinfagnani @FredKSchott ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@usergenic might also know.

Choose a reason for hiding this comment

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

If the source file is imported somewhere with a string literal identifier (i.e. not by concatenation or variable reference) then you shouldn't need them here. However this is good if your source filenames can not be statically resolved by build.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to revert this then - it doesn't seem to be affecting anything yet. Also, I talked w/ @usergenic off of GitHub and he said that shell is one of the roots of this graph and dynamic imports are considered when finding other sources. Given that src/my-app.js eventually gets to all of the files in src through both regular and dynamic imports, it should be sufficient to get the whole graph. This should be easy to add back later if we find it's better.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@@ -79,14 +79,14 @@ Run `polymer help build` for the full list of available options and optimization

This command serves your app. Replace `build-folder-name` with the folder name of the build you want to serve.

polymer serve build/build-folder-name/
npm start build/build-folder-name/
Copy link
Contributor

Choose a reason for hiding this comment

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

is this true? i.e. have you tested this

Copy link
Contributor

@abdonrd abdonrd Apr 19, 2018

Choose a reason for hiding this comment

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

Yep!

screen shot 2018-04-19 at 14 41 56

Copy link
Member Author

Choose a reason for hiding this comment

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

@abdonrd added these as npm scripts in package.json, they use the cli there, and I think this means that you don't have to install the cli globally to run through these instructions anymore.

Copy link
Contributor

@abdonrd abdonrd Apr 19, 2018

Choose a reason for hiding this comment

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

@bicknellr exactly! Use the Polymer CLI installed in the project.
It helps to avoid install the Polymer CLI globally. And more now that it is in alpha.

Also the users can use the common npm start, npm test, etc scripts.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍


<!-- Load your application shell -->
<script type="module" src="./src/my-app.js"></script>
<script type="module" src="src/my-app.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

why are the relative paths wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we set it like this and use the base tag to support the PRPL Server?

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this change makes a difference since it's still a relative URL that uses the base tag. No action needed.

src/my-app.js Outdated
subject to an additional IP rights grant found at http://polymer.github.io/PATENTS.txt
*/
* @license
* Copyright (c) 2018 The Polymer Project Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

blehhhh pinging @azakus to double check we should be bumping this year (i guess it's technically a new file?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Substantial edits have been made, so updating is fine.

Copy link
Member

Choose a reason for hiding this comment

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

Only new new files need a new date AFAIK

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed the dates back.

setPassiveTouchGestures(true);

setRootPath(Polymer.rootPath);
Copy link
Member Author

Choose a reason for hiding this comment

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

Polymer doesn't seem to be getting imported from anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's set as a global in index.html. I'm going to skip doing anything about that in this PR but I think these might make sense to move out to index.html too? I've opened a branch (global-settings) to look at this.


### Build

The `polymer build` command builds your Polymer application for production, using build configuration options provided by the command line or in your project's `polymer.json` file.
The `npm run build` command builds your Polymer application for production, using build configuration options provided by the command line or in your project's `polymer.json` file.

You can configure your `polymer.json` file to create multiple builds. This is necessary if you will be serving different builds optimized for different browsers. You can define your own named builds, or use presets. See the documentation on [building your project for production](https://www.polymer-project.org/2.0/toolbox/build-for-production) for more information.
Copy link
Member Author

Choose a reason for hiding this comment

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

src/my-app.js Outdated
</iron-pages>
</app-header-layout>
</app-drawer-layout>
`;
}

static get is() { return 'my-app'; }
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think the is getter is necessary anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@bicknellr
Copy link
Member Author

bicknellr commented Apr 20, 2018

FWIW, I think googlebot was worried that my merge commit which has my personal email attached to it (thanks to GitHub not letting me choose which to use on which repos) is somebody new.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@bicknellr
Copy link
Member Author

Chill out googlebot, it's still just me.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@bicknellr bicknellr merged commit 44bccb4 into 3.0-preview Apr 21, 2018
@bicknellr bicknellr deleted the polymer-3-updates branch April 21, 2018 01:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants