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

Support Rollup and ES5 #31

Closed
a-teammate opened this issue Jan 24, 2020 · 9 comments
Closed

Support Rollup and ES5 #31

a-teammate opened this issue Jan 24, 2020 · 9 comments
Labels
enhancement New feature or request

Comments

@a-teammate
Copy link

a-teammate commented Jan 24, 2020

First of all thanks for the lib :)
It looks very promising, but I struggle getting it to work in my rollup based project where I use ES5 modules.

Disclaimer: Maybe I am missing something essential, as I am new to this whole js module mess.
However, other libraries work out of the box for me after importing them in a sapper project (i.e. nodejs framework for svelte). So that is why I still figure this could be an issue in the wildcard-api package.

I do the module conversion with the below rollup.config.js using the commonjs and node-resolve packages.

I played around with the preferBuiltins args of resolve() and commonjs(), but no luck..

It keeps complaining about: Error resolving module specifier: util in the client
after calling the endpoint id = await wildcard.endpoints.getHelloWorldId();
(Sidenote: also import {endpoints} from '@wildcard-api/client'; as used in the examples, does not work for me, only when I do import wildcard from '@wildcard-api/client'; and use its endpoints property).

Also the build prints the warnings:

'util' is imported by node_modules/@brillout/assert/log.js, but could not be resolved – treating it as an external dependency
'util' is imported by util?commonjs-external, but could not be resolved – treating it as an external dependency
'util' is imported by node_modules/reassert/log.js, but could not be resolved – treating it as an external dependency

But when I add && typeof window === 'undefined' to @brillout/assert/utils/isNodejs.js and reassert/utils/isNodejs.js, those build warnings go away, but not the error caught by the client

// rollup.config.js
import resolve from 'rollup-plugin-node-resolve';
import replace from '@rollup/plugin-replace';
import commonjs from 'rollup-plugin-commonjs';
import svelte from 'rollup-plugin-svelte';
import sveltePreprocess from 'svelte-preprocess';
import babel from 'rollup-plugin-babel';
import { terser } from 'rollup-plugin-terser';
import config from 'sapper/config/rollup.js';
import pkg from './package.json';

const mode = process.env.NODE_ENV;
const dev = mode === 'development';
const legacy = !!process.env.SAPPER_LEGACY_BUILD;

const onwarn = (warning, onwarn) => (warning.code === 'CIRCULAR_DEPENDENCY' && /[/\\]@sapper[/\\]/.test(warning.message)) || onwarn(warning);
const dedupe = importee => importee === 'svelte' || importee.startsWith('svelte/');

export default {
    client: {
        input: config.client.input(),
        output: config.client.output(),
        plugins: [
            replace({
                'process.browser': true,
                'process.env.NODE_ENV': JSON.stringify(mode)
            }),
            svelte({
                dev,
                hydratable: true,
                emitCss: true,
                preprocess: sveltePreprocess({ postcss: true }),
            }),
            resolve({
                browser: true,
                dedupe,
                preferBuiltins: true,
                customResolveOptions: {

                    moduleDirectory: 'node_modules'

                }
            }),
            commonjs({}),

            legacy && babel({
                extensions: ['.js', '.mjs', '.html', '.svelte'],
                runtimeHelpers: true,
                exclude: ['node_modules/@babel/**'],
                presets: [
                    ['@babel/preset-env', {
                        targets: '> 0.25%, not dead'
                    }]
                ],
                plugins: [
                    '@babel/plugin-syntax-dynamic-import', ['@babel/plugin-transform-runtime', {
                        useESModules: true
                    }]
                ]
            }),

            !dev && terser({
                module: true
            })
        ],

        onwarn,
    },

    server: {
        input: config.server.input(),
        output: config.server.output(),
        plugins: [
            replace({
                'process.browser': false,
                'process.env.NODE_ENV': JSON.stringify(mode)
            }),
            svelte({
                generate: 'ssr',
                dev,
                emitCss: true,
                preprocess: sveltePreprocess({ postcss: true }),
            }),
            resolve({
                dedupe,
                preferBuiltins: true,
            }),
            commonjs({preferBuiltins: true})
        ],
        external: Object.keys(pkg.dependencies).concat(
            require('module').builtinModules || Object.keys(process.binding('natives'))
        ),

        onwarn,
    }
};

On another note, there are more warnings when building the client:

Circular dependency: node_modules/@brillout/format-text/index.js -> node_modules/reassert/warning.js -> node_modules/reassert/assert.js -> node_modules/@brillout/format-text/index.js
Circular dependency: node_modules/@brillout/format-text/index.js -> node_modules/reassert/warning.js -> node_modules/reassert/assert.js -> /home/malte/Dokumente/zeug/stage-2/svelte-start/node_modules/@brillout/format-text/index.js?commonjs-proxy -> node_modules/@brillout/format-text/index.js
Use of eval is strongly discouraged, as it poses security risks and may cause issues with minification
67:         return str.length;
68:     }
69:     const stringWidth = eval('require')('string-width');
                            ^
70:     return stringWidth(str);
71: }
Use of eval is strongly discouraged, as it poses security risks and may cause issues with minification
21:   // we use `eval('require')` instead of `require` to
22:   // make sure that webpack doesn't bundle `node-fetch`
23:   fetch = eval('require')('node-fetch');
              ^
24: }

EDIT:
The above errors disappeared after using the rollup plugins

import builtins from 'rollup-plugin-node-builtins';
import globals from 'rollup-plugin-node-globals';

for the client. However now the client simply catches the error:
"require is not defined"

@brillout
Copy link
Owner

Hi @a-teammate,

I just tried and yes Rollup is having difficulties with circular dependencies.

Some circular dependencies come from code written by myself but some come from libraries that I didn't write. At this point I'm not willing to change all my circular dependencies and open an issue of each library that has some as well.

Sapper also works with webpack, would using webpack instead of Rollup be an option for you?

I'm curious; why did you go for Rollup instead of webpack in the first place?

First of all thanks for the lib :) It looks very promising

Thanks :-)

@a-teammate a-teammate changed the title Support Rollup and ES6 Support Rollup and ES5 Jan 25, 2020
@a-teammate
Copy link
Author

I just tried and yes Rollup is having difficulties with circular dependencies.

Do you think that is the issue why it is not working at all?
I'd have thought the issue is that the common.js translator plugin does not find some of the dependencies of the lib. But true that, could be bc of circular deps :/ (Reading This it looks like the commonjs translator really seems to have problems with circular deps)

Coming from other languages, for me it often made sense to refactor circular dependencies though (even in languages like python, where the module system is much more advanced in comparison to js/node, but especially in languages like C++ or Java).

I'm curious; why did you go for Rollup instead of webpack in the first place?

Actually mostly bc allmost all tutorials and introductions for sapper were built up on rollup (guess thats bc the creator of svelte and sapper is also a contributor to rollup) and secondly because the api seems a bit easier to me. Thirdly (but at least I tried to blend that out) because of the hype articles about rollup when webpack 2 wasn't released and it was the one giving huge results in bundle sizes (that is really dare to me, hence my choice of svelte instead of something traditional).

Sapper also works with webpack, would using webpack instead of Rollup be an option for you?

Hm I thought of it already, but I do not see that solving the root-cause of my problem, which is the fucked up ecosystem of incompatible node-packages :D (i know that is just historical, rooting from before ES5 modules). Out of curiousity, why is wildcard-api using commonjs modules? Also historical? Or does it have other reasons?

So if I switch, I'll do the switch to bazel, in order to have more control over my dependencies and be able to patch them myself if needed. As there is simply no option here offering "convenience", or would you say webpack is convenient from your experience? (Havent tried it yet) :)

As I'm new to this whole ecosystem, this was just a thing that surprised me: the huge demand of build steps and transpilers (for a "scripting language"), without good tools for that (no offence rollup and webpack with all their plugins, but still: "do you work out of the box for a simple poc"?).

But that is probably already too much off-focus atm, as I'm only prototyping for now.

So probably I'll try to patch wildcard-api and its dependencies for one or two days (probably some assert, formattext functions are not needed when using ES6) and if I'm too novice / too dumb for that, I'll check out some alternatives.

@brillout
Copy link
Owner

Hallo Malte :-)

Hm I thought of it already, but I do not see that solving the root-cause of my problem, which is the fucked up ecosystem of incompatible node-packages :D

Actually it does, virtually all packages are compatible with webpack nowadays.

Out of curiousity, why is wildcard-api using commonjs

Because it's convenient not to have to transpile for running in Node.js. E.g. Wildcard's tests don't need a transpilation step to run.

I'll rewrite everything with ES modules once Node.js supports ES modules (without flag).

without good tools

Do you know Parcel https://parceljs.org/? It does a good job at working out of th box without any configuration.

bundle sizes (that is really dare to me

You may like Goldpage: https://github.com/reframejs/goldpage. Wiht Goldpage, you can implement pages that have zero browser-side JavaScript.

Goldpage takes care of all the building for you. It's based on webpack so you won't have any problems loading libraries.

@a-teammate
Copy link
Author

I'll rewrite everything with ES modules once Node.js supports ES modules (without flag).

Actually node 13 does enable ES module support by default.

Do you know Parcel https://parceljs.org/? It does a good job at working out of th box without any configuration.

Good tip. I have known about it, but I don't remember the reasons I discarded it. But zero config should have been higher weighted in my decision. Also fast builds should have been higher weighted when doing prototyping..
Made it on my Todo Short-list.

You may like Goldpage: https://github.com/reframejs/goldpage. Wiht Goldpage, you can implement pages that have zero browser-side JavaScript.

Actually my app is kind of the opposite of a static webpage, its more chat-app like. But since mobile is so important I still try to keep the footprint down as I go.

Thank you very much for the advises!

@brillout
Copy link
Owner

node 13 does enable ES module

Neat. I'll consider re-writing everything with ES modules then.

I'll try to patch wildcard-api

Any luck on that? We can do it together if you want; let me know how things go and I'll help whenever I can.

its more chat-app like

I see. If you don't need SSR then yea don't use Goldpage.

But since mobile is so important I still try to keep the footprint down as I go.

I know that svelte is marketing itself as mobile performant. Does svelte deliver on that promise? I'm curious.

Thank you very much for the advises!

I like our discussion :-)

@a-teammate
Copy link
Author

a-teammate commented Jan 26, 2020

Any luck on that? We can do it together if you want; let me know how things go and I'll help whenever I can.

I can fix the build errors by replacing all calls to the @brillout/assert lib with that mock:

class assert {
    static internal(param1, ...args) {
        console.assert(param1);
    }
    static usage(param1, ...args) {
        console.assert(param1);
    }
};

I had to do that in the files wildcard-api/client/WildcardClient.js, wildcard-api/client/makeHttpRequest.js wildcard-api/client/WildcardClient.js @brillout/json-s/src/reviver.js
In @brillout/fetch/index.js not your assert lib, but the reassert lib is used, I just replaced that with the same stub code.

Problem: the hello world is not yet working, but the client is also not throwing errors anymore, but maybe that is a bad thing. Log remains empty though.
But as I also don't test it in isolation yet, but in my sapper app, combining sapper.middleware(); and wildcardMiddleware() for express.

EDIT:

I know that svelte is marketing itself as mobile performant. Does svelte deliver on that promise? I'm curious.

Honestly I didnt verify it yet. It seems performant by my feel, but I've not crunched any numbers. What I like is how reactivity is baked into the framework and how you write components in general (referencing js vars from html, foreach, such stuff) maybe also a bit more than vue.

@brillout
Copy link
Owner

brillout commented Feb 5, 2020

Hi!

I only read your reply today- somehow I skipped the GitHub notification, sorry for the late reply!

I can have a look at it myself, do you have a reproduction code I could use?

@brillout brillout added the enhancement New feature or request label Sep 16, 2020
@brillout
Copy link
Owner

brillout commented Oct 7, 2020

Closing this - I made many changes and this may be fixed by now.

@brillout brillout closed this as completed Oct 7, 2020
@brillout
Copy link
Owner

brillout commented Oct 7, 2020

Let me know if you still run into problems.

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