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

Crawl imports #13

Open
bluwy opened this issue Dec 27, 2022 · 5 comments
Open

Crawl imports #13

bluwy opened this issue Dec 27, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@bluwy
Copy link
Owner

bluwy commented Dec 27, 2022

Crawling imports can enable linting more files that may have exports issues, e.g. a .mjs file imports an ESM .js file but is treated as CJS. This would catch errors for sveltejs/kit#8267.

This would also enable catching packages that uses nodejs builtin modules for browser exports. (NOTE: We may need a setting to tell if this package is from node or browser by default)

@bluwy bluwy added the enhancement New feature or request label Dec 27, 2022
@thepassle
Copy link

Piggy backing off of this discussion on twitter: https://twitter.com/passle_/status/1626222241091579904?s=20

It could be good to also flag other non-standard imports, like:

import css from './styles.css';
import data from './data.json';

and potentially even other non-standard, conventional-ey, magic-ey imports?

Additionally, it seems like this feature would be a great usecase for es-module-lexer 🙂

@bluwy
Copy link
Owner Author

bluwy commented Feb 17, 2023

Yeah that would be good to check too. I'm not sure if it needs to be flagged always as a package/entrypoint can be advertised to only work with bundlers. It goes a bit in hand with #19, but I have some ideas to conditionally raise this forward.

Re es-module-lexer, I'd also like to lex CJS require() (best effort) but cjs-module-lexer doesn't do that. Might be time to yak shave one.

@thepassle
Copy link

Perhaps it should be made configurable then, imo (and this is where the tool becomes opinionated 😄 ) packages shouldn't publish non-standard imports by default. If a package advertises to be used with a bundler, that should be opt-in behavior.

Good point regarding es-module-lexer. I've previously used TS for collecting the imports/exports in a file:
https://github.com/open-wc/custom-elements-manifest/blob/master/packages/analyzer/src/features/collect-phase/collect-imports.js
https://github.com/open-wc/custom-elements-manifest/blob/master/packages/analyzer/src/features/analyse-phase/exports.js

It could be made to work with commonjs/require, too.

The benefit of using TS (over for example acorn or babel) to do the static code analysis for imports/exports is that it's pretty forgiving and doesn't error on unexpected syntax. This is nice, because it means less maintenance work whenever new syntax is introduced to the language. (This is something that unpkg.com currently struggles with, it uses babel to transform module specifiers on the fly, but that errors on new syntax, like optional chaining, and causes unpkg to just break for packages that use unsupported syntax)

@bluwy
Copy link
Owner Author

bluwy commented Feb 17, 2023

Yeah I was thinking there would be an opt-in option to tell publint that this package, by default, works with bundlers/node/browser only. And if the option is not specified, it would mention something like "this entrypoint works with bundlers only".

Thanks for the tip about using TS! I might try that as a last resort since it increases the package size quite a bit, and it'd have to work in the browser too. I've got a good base to start with (https://github.com/bluwy/fmu) so I'll probably work from there. (I saw you were working something similar on Twitter too)

@thepassle
Copy link

Yeah, TS will indeed increase bundlesize by quite a bit. It does run in the browser however, which is nice (here's an example of a tool I built that uses TS compiler to create an AST and analyze it, it runs fully in the browser: https://custom-elements-manifest.netlify.app/) 🙂 It does require some build magic to bundle it correctly for the browser, however. If you do end up going down that road I'd be happy to share some findings, but it seems like fmu will probably do the trick 😄 (Thanks for linking that btw! Im currently learning Rust, so its very helpful to take a look at different implementations)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants