-
Notifications
You must be signed in to change notification settings - Fork 17
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
missing main entry #23
Comments
Hmm ... the But also a |
I just made double sure that at least |
Any idea how to resolve problem? What would the published |
There two degrees of freedom
And only there entry points
Let me dig around How is this handled in the other @octokit modules ? For my node cli app I want to pick node/esm (through @octokit/rest) |
Hmm the other Octokit modules mostly work the same in both Node & browser, with the exception of https://github.com/octokit/auth-app.js/ which is using the browser's WebCrypto API and for Node it's using the I did not yet migrate all @octokit modules, but soon all of them will be built with https://github.com/pikapkg/pack and will have the separate builds for node (using Pika also distributes a |
The problem is I am only using import octokit from "@octokit/rest"; |
Ah got it. Hmmm I really don't know, but I hope we find a solution for this. I asked the folks from Pika on twitter, maybe they have an idea what to do: https://twitter.com/gr2m/status/1169357730944958464 |
when declaring universal-user-agent as external in rollup i am able to select the node content out of the package (even through @octokit/rest). |
It should be as of |
@arlac77 I'm trying to use
I'm using the latest |
@schof I have no solution only a workaround my list of to be excluded modules (rollup) is getting longer and longer. // rollup.config.js
const external = [
...builtins,
"universal-user-agent",
"@octokit/rest",
"node-fetch"
]; |
@arlac77 I tried the same thing you are doing with similar results. Ultimately I got things working by not excluding anything and monkey patching the resolve: {
// Monkey patch to resolve https://github.com/mobileposse/neptune/issues/296
modules: [path.resolve(__dirname, 'build/src/monkey-patch'), 'node_modules'],
extensions: ['.tsx', '.ts', '.js']
} then i took the
Hope that helps. Good luck! |
I’m sorry this is causing so much headache, I hope we find a simpler solution that I can apply to the libraries, universal-user-agent is not the only one that is published this way |
Hey yall, sorry for the delay jumping into this! Happy to help where i can. @arlac77 sounds like you summed it up pretty well, we have a 2x2 matrix and only 3 entrypoints to represent them with. On top of that, "module" has a real frontend focus at this point, since web bundlers are primarily the only real environment that they are expected to be used in. The result is that "Node ESM" is relatively unsupported, given that the official plan is for Node to move to: Like @gr2m I'm still not sure I understand what situation is wanting you to use rollup inside of Node. Are you trying to bundle a Node application into a single JS file? If so, Webpack would probably be a better fit since it's much more Common.js dependent & reliant. https://github.com/zeit/ncc & https://github.com/zeit/pkg may also be good options for you. If you really do need to use Rollup, have you tried the latest version of https://github.com/rollup/rollup-plugin-node-resolve ? Its most recent syntax allows you to define either |
@FredKSchott, @gr2m : The |
@maplesteve There must be a way in webpack to workaround this, like there is in rollup as mentioned by @FredKSchott. It's not something that I can fix in Octokit itself, I'm afraid. But people keep running into this so I'd love to address it in the READMEs. |
Could you please try to directly edit Also, could you please setup a repository with the most minimal build setup that reproduces the problem for you? I would appreciate to have both a setup using webpack and rollup. Let's fix this 💪 |
There is also a new "exports" field that might work: I've asked on the twitters and Myles shared some helpful links: |
Here a starting point showing rollup selecting browser content by default and the 'external' workaround to exclude 'universal-user-agent' from bundling |
I’m starting to think this may be a Lambda misconfiguration/issue, If they’re using webpack internally there’s a configuration for this that they could probably set to solve for this: https://webpack.js.org/configuration/resolve/#resolvealiasfields |
@gr2m
|
@FredKSchott regarding Lambda: "they" in terms of AWS are doing nothing with webpack. Webpack is used to bundle the code which then gets uploaded and used in the Lambda environment. It worked before. |
@maplesteve I found netlify/netlify-lambda#138 which sounds similar to yuor probelem. See their fix at https://github.com/netlify/netlify-lambda/pull/160/files. It looks like I think Shawn summerizes the problem pretty well
|
@maplesteve ah gotcha, I’d misunderstood. If you own the webpack config / are running webpack yourself, hopefully that resolveAlias config option fixes thing |
The webpack changes according to the netlify issue did not help. I created a repo to reproduce: https://github.com/maplesteve/navigator-error |
Can you please try to set module.exports = {
resolve: {
aliasFields: []
}
}; |
@gr2m tried your suggestion re: |
I think I hit this issue when trying to add webpack to the actions/typescript-action template. This is a minimal //import * as github from '@actions/github';
const github = require('@actions/github');
export async function run() {
try {
console.log("HELLO WEBPACK!");
var slug = ['user','repo']
if (github.context.payload.repository && github.context.payload.repository.full_name) {
slug = github.context.payload.repository.full_name.split('/')
}
}
catch (error) {
throw error;
}
}
run(); And the const join = require('path').join;
module.exports = {
entry: './dist/main.js',
target: 'node',
output: {
path: join(__dirname, 'dist'),
filename: 'main.pack.js'
},
optimization: {
minimize: false
},
devtool: 'sourcemap'
}; Either requiring or importing /***/ "./node_modules/universal-user-agent/dist-web/index.js":
/*!*************************************************************!*\
!*** ./node_modules/universal-user-agent/dist-web/index.js ***!
\*************************************************************/
/*! exports provided: getUserAgent */
/***/ (function(module, __webpack_exports__, __webpack_require__) {
"use strict";
__webpack_require__.r(__webpack_exports__);
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "getUserAgent", function() { return getUserAgent; });
function getUserAgent() {
return navigator.userAgent;
} As a result, the action fails:
EDIT This ugly workaround fixes it: eine/tip@f603695 |
Here another type of workaround: rollup.config.js import virtual from "rollup-plugin-virtual";
...
plugins: [
virtual({
"universal-user-agent":
"export function getUserAgent() {return `Node.js/${process.version.substr(1)} (${process.platform}); ${process.arch})`}"
}),
...otherPlugins
] |
@gr2m there is a recently updated proposal for Conditional Mapping in packages |
Thanks I'm following it closely, relevant discussion is here: nodejs/modules#401 |
Does anyone know of a way to workaround this (other than editing the I've tried using
But that causes other |
There is no ESM version of I've worked with Node's ES Modules working group over the past weeks and there is a clear way forward now. It will take some time for the build tools and Pika to catch up, but Node's latest spec supports all that we need: https://github.com/nodejs/node/blob/master/doc/api/esm.md The package.json will look like this: {
"main": "dist-node-cjs/index.js",
"module": {
"browser": "dist-browser/index.js"
"default": "dist-node/index.js"
}
} Once the tooling catches up, I'll update the library. Shouldn't take too long, it's mostly Pika and its Rollup plugins, and Fred from Pika has been involved in the same working group. |
I've managed to use Webpack's plugins: [
new webpack.NormalModuleReplacementPlugin(
/universal-user-agent\/dist-web\/index\.js/,
"../dist-src/index.js"
)
] I used |
The above workaround with resolve: {
// ...
alias: {
"universal-user-agent": path.resolve(
__dirname,
"node_modules/universal-user-agent/dist-src/index.js"
)
}
} |
I'll add a try/catch around the Once conditional exports are supported by Any thoughts? |
I've decided to greatly simplify this package to workaround the build problems y'all are experiencing. I was hoping for conditional exports to be adopted more quickly, but it does not look like it. See #52 |
🎉 This issue has been resolved in version 6.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This issue has been resolved in version 6.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
since version 4.0.0 rollup picks the browser.js instead of node.js
maybe package main entry got lost in 34fedff ?
The text was updated successfully, but these errors were encountered: