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

Improve .npmignore of xterm-addon-search #3138

Closed
aeschli opened this issue Oct 21, 2020 · 7 comments · Fixed by #3476
Closed

Improve .npmignore of xterm-addon-search #3138

aeschli opened this issue Oct 21, 2020 · 7 comments · Fixed by #3476
Labels
area/build Regarding the build process good first issue help wanted type/bug Something is misbehaving
Milestone

Comments

@aeschli
Copy link

aeschli commented Oct 21, 2020

You can see from
https://unpkg.com/browse/xterm-addon-search@0.7.0/

that the xterm-addon-search npm packages contains unnecessary resources:

@aeschli
Copy link
Author

aeschli commented Oct 21, 2020

@jerch
Copy link
Member

jerch commented Oct 25, 2020

Partly also covered by the issue #3137. Guess we should clarify, what we want to end up in packages for whatever reason and exclude everything else.

@jerch jerch added area/build Regarding the build process type/automation Relating to CI/CD pipeline, automation, etc. labels Oct 25, 2020
@Tyriar Tyriar added type/bug Something is misbehaving good first issue help wanted and removed type/automation Relating to CI/CD pipeline, automation, etc. labels Oct 29, 2020
@kumaran-14
Copy link
Contributor

I would like to take on this and related #3137
What would be the solution to this?

@Tyriar
Copy link
Member

Tyriar commented Mar 17, 2021

@kumaran-14 the solution for these is to make the .npmignore for the various modules (addons+code for #3137) exclude files that we should not be shipping inside the modules. You might want to read up about how .npmignore works before jumping in - it's similar to .gitignore.

@jerch
Copy link
Member

jerch commented Mar 21, 2021

Had another look into this - it seems our addon packages are stuck between source distribution - nodejs target - browser prebundled, we kinda have abit of everything but nothing to proper full extend 😸

Not sure where to go from here, a few thoughts are:

  • A source package distribution never gonna work to full extend with addon packages due their entanglement with the base repo. Furthermore full source can also be obtain from the repo itself, so src is not that useful.
  • If we want to go with one npm package only, we prolly should move browser builds to dist and let main point to the addon entry file (addon class). This would be my preferred variant, it is just moving current content abit around.
  • If we want browser builds to be distributed via npm, we should do a second package with main pointing to an ES6 module bundle of the addon. Those will not work in nodejs out-of-the-box, but the browser will be more than happy.

@mofux
Copy link
Contributor

mofux commented Mar 22, 2021

If we want browser builds to be distributed via npm, we should do a second package with main pointing to an ES6 module bundle of the addon. Those will not work in nodejs out-of-the-box, but the browser will be more than happy.

AFAIK we could use themodule field in package.json to point to the entry file of the ES6 Module bundle. The main should still point to the CJS version.

@jerch
Copy link
Member

jerch commented Apr 27, 2021

To solve the build target ambiguity, there is a new package.json entry exports since nodejs v12 (see here for an introduction). With this we can distribute ES6 builds together with default nodejs build (commonjs) in just one package. It is even possible to put an IIFE build as third goodie in the package so old style non ES6 script tags still work.

@Tyriar Tyriar added this to the 4.15.0 milestone Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Regarding the build process good first issue help wanted type/bug Something is misbehaving
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants