Polymer / tools

Polymer Tools Monorepo
BSD 3-Clause "New" or "Revised" License
430 stars 200 forks source link

The linter's integration test throws warnings from trouble analyzing .ts files in hydrolysis (from bower). #1481

Open rictic opened 7 years ago

rictic commented 7 years ago

There's a good chance that this is just an issue with hydrolysis publishing .ts files to bower in a strange state or something, but in the linter's integration test we pull down a ton of code from bower and run analyzePackage over the bower_components dir itself. As of alpha.23 it prints warnings like:

declare module 'escodegen' {
~

hydrolysis/custom_typings/escodegen.d.ts(0,0) error [unable-to-analyze] - Unable to analyze file: Cannot read property 'length' of null

declare module 'espree' {
~

hydrolysis/custom_typings/espree.d.ts(0,0) error [unable-to-analyze] - Unable to analyze file: Cannot read property 'length' of null

declare module 'estraverse' {
~

hydrolysis/custom_typings/estraverse.d.ts(0,0) error [unable-to-analyze] - Unable to analyze file: Cannot read property 'length' of null

/**
~

hydrolysis/src/analyzer.ts(0,0) error [unable-to-analyze] - Unable to analyze file: Cannot read property 'length' of null
rictic commented 7 years ago

Ok, looked into this a bit more. We're running into module resolution issues in the typescript analyzer. hydrolysis's ts has imports like:

// from url-resolver.ts
import * as resolver from './resolver';

One issue is a simple bug that resolves that module reference to url-resolver/url-resolver.ts/resolver rather than url-resolver/resolver. The other, trickier issue is that even after that, that module specifier doesn't have a file extension. That's not fatal, as it looks like there's supposed to be some component that finds the right file extension to use, but I don't think we have it hooked up, so the typescript analyzer crashes because it can't determine the file extension.

justinfagnani commented 7 years ago

Thanks for looking into that. The way I wrote the JS and TS import scanners assumes web-compatible module specifiers - that they actually resolve to a file. We don't support node style resolution or even implicit extensions. That could be made more clear somewhere in the docs.

Given that support for modules is slowly coming along and not advertized, I don't think source with JS imports should be in our integration tests yet.

rictic commented 7 years ago

Fair. I'm happy to mark all of the hydrolysis files as ignored in the linter's integration test. (its integration "test" consists of bower installing a ton of polymer elements, then trying to lint them. it's also only advisory at this point, it won't fail continuous integration, but it's helpful when writing new lint rules to see if/when our own code fails it).

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.