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

Remove node_modules from published extension? #91

Open
strlns opened this issue Mar 5, 2023 · 7 comments
Open

Remove node_modules from published extension? #91

strlns opened this issue Mar 5, 2023 · 7 comments

Comments

@strlns
Copy link
Collaborator

strlns commented Mar 5, 2023

Hi, I'm not sure if I am completely misunderstanding something here, but it seems like you are including your node_modules folder in the published extension, which - AFAIK - should never be necessary.

https://code.visualstudio.com/api/working-with-extensions/bundling-extension#run-webpack

Related:
Viijay-Kr/typescript-cleanup-definitions#1 (comment)
#90 (comment)

I'm not sure if or why this is necessary?

I'll look if I can provide a PR for this later.

Originally posted by @strlns in #89 (comment)

@strlns strlns changed the title Add pre-publish script Check pre-publish script? Mar 5, 2023
@strlns strlns changed the title Check pre-publish script? Remove node_modules from published extension? Mar 5, 2023
@Viijay-Kr
Copy link
Owner

Viijay-Kr commented Mar 6, 2023

@strlns Great Observation , the node modules folder wasn't shipped until we introduced a typescript server plugin to resolve #68 .

The plugin is enabled by the extension and is shipped with the node_modules hence it shouldn't be ignored by .vscodeignore.

The plugin filters our definition results from the declaration file provided by the starter kits like vite ,next, cra etc

Checkout the source code of Styled Components extension.

https://github.com/styled-components/vscode-styled-components/blob/main/.vscodeignore

Also there is a specific commit by one of the contributors to not ignore node_modules.

One alternative I see is to encourage our users to use the plugin directly inside their tsconfig.json.

If there is another alternative to this then I'd be happy to have a PR

@strlns
Copy link
Collaborator Author

strlns commented Mar 6, 2023

Hi @Viijay-Kr, thank you for your response and indeed I also found node_modules in the published package of the styled components extension.

I'm not sure if I already understand how VSCode extensions hook into the TS language server without using a workspace TS version.

I'll check out some more documentation on this as I don't want to jump to invalid conclusions.

Casually observed, this package here still weighs a lot more than the packaged extension at https://marketplace.visualstudio.com/items?itemName=styled-components.vscode-styled-components. This just makes me wonder if build dependencies are shipped.

The deprecation warning on that other package page confuses me quite a bit, as the link is circular.

I'll leave this open until I understand the problem better, thank you for the hints.

@karlhorky
Copy link
Collaborator

In case it fits the project goals of the React CSS modules extension, there would be ways to avoid including the node_modules, such as bundling the code before publishing (eg. using esbuild or something like ncc)

@strlns
Copy link
Collaborator Author

strlns commented Mar 6, 2023

@karlhorky if I understand @Viijay-Kr correctly, the added TS language server plugin is the reason why node_modules is included, see

"typescriptServerPlugins": [
(where the plugin is registered).

The code is already bundled via Webpack and the main file loaded by VSCode is the compiled file dist/extension.js.

"main": "./dist/extension.js",

We'd need a way to reference bundled code in the line of my first link instead of using Node module resolution.

Or prune the bundled node_modules to only include the relevant module.

Currently it seems like everything from dependencies (not devDependencies) is included.

Not sure if that was helpful, currently I'm not sure if the other entries there are needed at runtime in VSCode context or if they are already bundled by webpack into the dist bundle.

@Viijay-Kr
Copy link
Owner

@strlns Only the TS plugin is needed inside the node_module. The rest is not needed. So moving the other dependencies to dev depending should minimize the load on node_modules

@strlns
Copy link
Collaborator Author

strlns commented Mar 6, 2023

Bit of a tradeoff here 🤔
Since I'm usually a fan of listing stuff that ends up in the bundle in dependencies, I don't want to change your dependency categorization.
If I understand correctly, this should be a job for .vscodeignore?

Sorry that I'm spending more time writing messages instead of submitting PRs. Currently have to stay in bed because of an injury and haven't got too much energy.

Maybe just add node_modules there and then unignore the TS plugin?

@Viijay-Kr
Copy link
Owner

If I understand correctly, this should be a job for .vscodeignore?

yes.

VS code expects us to ignore node_modules by default. So it doesn't matter if the other dependencies are moved to dev dependencies since they are dev mode only .

The extension is shipped as a production bundle using webpack with this one little caveat of including the plugin.

So @strlns I wouldn't stress to much on this one as the extension worked well before the introduction of the node_modules which was done after #70.

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

No branches or pull requests

3 participants