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

Convert CommonJS projects to es6 modules #62

Open
backspaces opened this issue Jun 16, 2019 · 18 comments

Comments

Projects
None yet
5 participants
@backspaces
Copy link

commented Jun 16, 2019

TLDR: This is a feature request to manage common js dependencies as well as es6 module dependencies.

Details:
If my dependencies includes a common js project, try converting it to es6 modules via https://github.com/rollup/rollup-plugin-commonjs (which is already in pika/web's package.json devDependencies).

It would be fine to limit this to explicit @pika/web.webDependencies rather than all of my package.json dependencies.

@FredKSchott

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

I'd love to know if this is possible. My understanding was that the plugin could understand common.js dependencies, but wouldn't support common.js entrypoints (the top-level webDependency package) because it couldn't get the export interface right without knowing what was being imported off of it. Do you know if that is true?

@backspaces

This comment has been minimized.

Copy link
Author

commented Jun 17, 2019

Hi Fred. OK, to test this, I used a mapbox module, osmtogeojson, in a new temporary repo. The repo has an npm install of the current rollup and rollup-plugin-commonjs, along with the test module osmtogeojson:

├── osmtogeojson@3.0.0-beta.3
├── rollup@1.15.6
└── rollup-plugin-commonjs@10.0.0

I initially used this with an html file using a script tag, to convert an osm (open street map) format to geojson to make sure it all works. It did.

So I then created an esm rollup via:

import commonjs from 'rollup-plugin-commonjs'

export default {
    input: './node_modules/osmtogeojson/osmtogeojson.js',
    output: {
        file: 'osmtogeojson.esm.js',
        format: 'es',
    },
    plugins: [commonjs({})],
}

I ran this, changing the script version of the html file to a <script type="module"> and used an import:

        <script type="module">
            import osmtogeojson from './osmtogeojson.esm.js'

It worked perfectly! Yay!

I've used this before with good results.

Is this what you were wondering about? Basically, many well formed modules using require/comonjs can apparently be converted to working es6 modules. I will say the sample size is small, but hopeful.

@backspaces

This comment has been minimized.

Copy link
Author

commented Jun 19, 2019

TLDR: I tried this on another es5 repo to make sure this is a reasonable approach, and it worked.

Details: Another example: chart.js. I submitted an issue to them about modules, and in particular, showed how to create an es6 module via the rollup idea above.
chartjs/Chart.js#5179 (comment)

Just to make sure the above would still work, I created a new repo, with npm init, and installed rollup & plugins as well as chart.js.

cjs@1.0.0 /Users/owen/Dropbox/src/tests/cjs
├── chart.js@2.8.0
├── rollup@1.15.6
├── rollup-plugin-commonjs@10.0.0
└── rollup-plugin-node-resolve@5.0.3

I used this rollup.config.json:

import commonjs from 'rollup-plugin-commonjs'
import resolve from 'rollup-plugin-node-resolve'

export default {
    input: 'Chart.js',
    output: {
        file: 'chart.esm.js',
        format: 'esm',
    },
    plugins: [resolve(), commonjs()],
}

I ran rollup via npx like this npx rollup -c which produced

Chart.js → chart.esm.js...
created chart.esm.js in 1.5s

Just to make sure it worked, I made a quick chart which worked.

<!DOCTYPE html>
<html>
    <head>
        <title>Test</title>
    </head>
    <body>
        <canvas id="chart"></canvas>
        <script type="module">
            import chart from './chart.esm.js'
            const canvas = document.getElementById('chart')
            new chart(canvas, {
                type: 'line',
                data: {
                    labels: [
                        'January',
                        'February',
                        'March',
                        'April',
                        'May',
                        'June',
                        'July',
                    ],
                    datasets: [
                        {
                            label: 'My First Dataset',
                            data: [65, 59, 80, 81, 56, 55, 40],
                            fill: false,
                            borderColor: 'rgb(75, 192, 192)',
                            lineTension: 0.1,
                        },
                    ],
                },
                options: {},
            })
        </script>
    </body>
</html>
@backspaces

This comment has been minimized.

Copy link
Author

commented Jun 19, 2019

So I think we're on to something here. I'd start with limiting this to explicit @pika/web.webDependencies rather than all of my package.json dependencies.

I don't know how to test for success. We can test for rollup errors. And maybe build a simple html file that just imports the module and signaling a failure to load somehow.

@krumware

This comment has been minimized.

Copy link

commented Jun 19, 2019

Over here crossing paths with you on chart.js. Already using chartjs in a project based on pikaweb (but the nasty old fashioned run around hacky way, per that chart.js issue). This would be amazing!

@backspaces

This comment has been minimized.

Copy link
Author

commented Jun 19, 2019

Hi @krumware, glad you're joining the conversation. I can use a sanity check!
Everyone: the Chart.js connection is here: chartjs/Chart.js#5179 (comment)

Did you ever try the rollup which uses the rollup-plugin-commonjs? I'd love feed back.

I did not look into the potential corner cases involving Moment for example. Moment has been converted to es6, but the issue is how would that work with the chart.esm.js mentioned above.

I think the answer here is that chart.js has a rollup, dist/Chart.bundle.js, which includes Moment.

So if we point pika/web to that, via @pika/web.webDependencies, then things should work. I'll give it a try.

@FredKSchott

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

TIL! This could be really great, thanks for doing the research on this!

Also because we use the commonjs plugin to handle internal, sub-dependencies, it looks like you can just point to common.js files (but not the package name) and @pika/web will already handle them, example:

"webDependencies": ["date-now/index.js"]
$ npx @pika/web
✔ @pika/web installed: date-now/index.js. [0.04s]

$ cat web_modules/date-now/index.js
var dateNow = now;

function now() {
    return new Date().getTime()
}

export default dateNow;
//# sourceMappingURL=index.js.map

I'm going to be on vacation for the next two weeks, and won't have regular access to internet. Which sucks timing wise, because this is really exciting. But It also sounds like there's not too much actual implementation work here, just thinking through the interface / how the user turns this on, is that correct? Open to any ideas here!

  • Do we just use the "main" entrypoint if "module" doesn't exist?
  • Do we give any heads up if we do that?
  • Agreed that this should start as opt-in "webDependencies" only. We are also exploring better auto-detecting via #68 / #49
@backspaces

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

Yay! Glad it works out, especially basically as-is. And thanks for pointing at the two other issues which merge nicely with this.

No worries about the timing, have a great, relaxing, insightful vacation.

@sdegutis

This comment has been minimized.

Copy link

commented Jun 21, 2019

@backspaces Hi, I'm from issue #68, hope you don't mind my joining this conversation. (Likewise I'd be curious about your thoughts in that issue too.)

So if I'm reading both of you correctly, the ability to bundle CJS modules such as "chart.js" and "osmtogeojson" is already available in @pika/web by just adding the main entry-point file (the same as its pkg.main) to pkg.[@pika/web].webDependencies?

@sdegutis

This comment has been minimized.

Copy link

commented Jun 21, 2019

This seems to work!

    "webDependencies": [
      "styled-components/dist/styled-components.browser.esm.js",
import styled from '/web_modules/styled-components/dist/styled-components.browser.esm.js';

Although it's verbose. Normally I would suggest an optional alternative "alias" syntax, such as:

    "webDependencies": [
      ["styled-components", "styled-components/dist/styled-components.browser.esm.js"],

Then we could just do this:

import styled from '/web_modules/styled-components.js';

However, I think the plan in #68 would solve this in a cleaner and better way: let Pika automatically scan imports in user code, finding the "/web_modules/styled-components.js" path and determining from this that it must compile the "styled-components" NPM module.

That said, Pika still wouldn't know exactly how to compile styled-components without first knowing tha it must find "dist/styled-components.browser.esm.js", which is not listed in styled-component's package.main:

  "main": "dist/styled-components.cjs.js",
  "module": "dist/styled-components.esm.js",

Those are both node-specific bundles, so styled-components may be an exception to this working smoothly. But in the general case, I think as long as Pika can find either "module" or "main" and correctly bundle it using rollup-plugin-commonjs as suggested by this issue, then this plan should work for a good majority of packages.

@sdegutis

This comment has been minimized.

Copy link

commented Jun 21, 2019

Ah, this trick of putting "styled-components/dist/styled-components.browser.esm.js" inside webDependencies doesn't work correctly for packages that import styled from 'styled-components'; directly, such as "styled-icons". As expected, @pika/web then looks for styled-components and finds "module": "dist/styled-components.esm.js" which is not the correct file.

Even though this is specific to styled-components, which was verified to be a strange exception, it's generally true about any package which doesn't have a proper pkg.module || pkg.main entry point.

I think as a reasonable workaround, users can let @pika/web know about potential "incorrect" packages like this, similar to the "alias" idea above. Maybe in conjunction with the plan in #68, instead of a whitelist, there would be an aliases entry:

  "@pika/web": {
    "aliases": {
      "styled-components": "styled-components/dist/styled-components.browser.esm.js",

Here, devs would be able to specify "real" entry points for broken packages, and Pika would be able to "alias" them in a configuration with Rollup, by saying "every time you see styled-components, either directly, or via another package which is requiring it, use this file instead as the entry-point."

@sdegutis

This comment has been minimized.

Copy link

commented Jun 21, 2019

I just verified that the concept itself works:

import commonjs from 'rollup-plugin-commonjs'
import resolve from 'rollup-plugin-node-resolve'

export default {
  input: [
    'styled-components',
    'styled-icons/fa-solid/Clipboard',
    'Chart.js',
    'react',
    'react-dom',
  ],
  output: {
    dir: 'out',
    format: 'esm',
  },
  plugins: [
    renameHardcodedTest(),
    resolve(),
    commonjs(),
  ],
};

function renameHardcodedTest() {
  return {
    name: 'rename-hardcoded-test',
    resolveId(src) {
      if (src === 'styled-components') {
        return 'node_modules/styled-components/dist/styled-components.browser.cjs.js';
      }
      return null;
    },
  };
}

This works even with the import of styled-components found nested inside of styled-icons, and the output Clipboard.esm.js file contains import styled from './styled-components.browser.cjs.js'; which is indeed the only styled-components file there is in the "out" directory.

The function renameHardcodedTest stands in place of the logic that would look at Object.entries(pkg['@pika/web'].aliases) and use that as the map.

I had to prefix the returned result with "node_modules" because otherwise Rollup wasn't able to find it. It seems that this is something the user should have to include in the aliased path themselves, because there's nothing magical about this prefix, it's just part of the path to the actual file that the user wants to include, starting at the project root.

@sdegutis

This comment has been minimized.

Copy link

commented Jun 21, 2019

(It's worth noting that the example in that comment uses @reactesm/react{,-dom} and installs it in react{,-dom}, because otherwise I couldn't get anything working with the actual React which isn't compatible with Rollup yet.)

@sdegutis

This comment has been minimized.

Copy link

commented Jun 25, 2019

One problem with this Rollup config is that the structure of the output directory doesn't match what we're importing:

out/
├── Chart.js
├── Clipboard.esm.js
├── chunk-5732a1e2.js
├── chunk-7be8ab94.js
├── react-dom.min.js
├── react.min.js
└── styled-components.browser.cjs.js

There's a few differences:

  • Often the filenames are different, such as the "react.js" in import '/web_modules/react.js' is different than the "react.min.js" that we actually see in the output directory.
  • The structure is different, such as how we have "Clipboard.esm.js" in the root of the output directory, whereas the input ("styled-icons/fa-solid/Clipboard") is gathered from a hypothetical import like import '/web_modules/styled-icons/fa-solid/Clipboard.js' which is nested.

When I add preserveModules: true, I do get a hierarchical tree, but I still don't get the structure that matches what the code hypothetically saw when scanning the imports:

out/
├── @emotion
│   ├── is-prop-valid
│   │   └── dist
│   │       └── is-prop-valid.esm.js
│   ├── memoize
│   │   └── dist
│   │       └── memoize.esm.js
│   └── unitless
│       └── dist
│           └── unitless.esm.js
├── Chart.js
│   └── dist
│       └── Chart.js
├── _virtual
│   ├── ReactPropTypesSecret.js_commonjs-proxy
│   ├── _commonjsHelpers.js
│   ├── checkPropTypes.js_commonjs-proxy
│   ├── factoryWithThrowingShims.js_commonjs-proxy
│   ├── factoryWithTypeCheckers.js_commonjs-proxy
│   ├── index.js_commonjs-proxy
│   ├── index2.js_commonjs-proxy
│   ├── index3.js_commonjs-proxy
│   ├── index4.js_commonjs-proxy
│   ├── moment.js_commonjs-proxy
│   ├── react-is.development.js_commonjs-proxy
│   ├── react-is.production.min.js_commonjs-proxy
│   ├── styled-components.browser.cjs.js
│   └── stylis.min.js_commonjs-proxy
├── is-what
│   └── dist
│       └── index.esm.js
├── memoize-one
│   └── dist
│       └── memoize-one.esm.js
├── merge-anything
│   └── dist
│       └── index.esm.js
├── moment
│   └── moment.js
├── object-assign
│   └── index.js
├── prop-types
│   ├── checkPropTypes.js
│   ├── factoryWithThrowingShims.js
│   ├── factoryWithTypeCheckers.js
│   ├── index.js
│   └── lib
│       └── ReactPropTypesSecret.js
├── react
│   └── react.min.js
├── react-dom
│   └── react-dom.min.js
├── react-is
│   ├── cjs
│   │   ├── react-is.development.js
│   │   └── react-is.production.min.js
│   └── index.js
├── styled-icons
│   ├── StyledIconBase
│   │   └── StyledIconBase.esm.js
│   ├── fa-solid
│   │   └── Clipboard
│   │       └── Clipboard.esm.js
│   └── node_modules
│       └── @emotion
│           └── is-prop-valid
│               └── dist
│                   └── is-prop-valid.esm.js
├── stylis
│   └── stylis.min.js
├── stylis-rule-sheet
│   └── index.js
└── tslib
    └── tslib.es6.js
  • Now we have "out/_virtual/styled-components.browser.cjs.js" instead of just "out/styled-components.browser.cjs.js"
  • We also still have "styled-icons/fa-solid/Clipboard/Clipboard.esm.js" which has the wrong filename (and ideally would be one directory higher but that point is confusing and slightly unrelated to this).
@sdegutis

This comment has been minimized.

Copy link

commented Jun 25, 2019

Regarding that last problem, Pika/web already seems to have the ability to do this, because it outputs modules matching the file/directory structure of webDependencies. But I can't for the life of me figure out where in the code this is happening. No output options seem relevant to it, none of the 5 plugins seem to make it happen. I'm just confused.

@sdegutis

This comment has been minimized.

Copy link

commented Jun 25, 2019

Got that working!

Turns out I needed to turn the array into a map:

  input: {
    'styled-components': 'styled-components',
    'styled-icons/fa-solid/Clipboard': 'styled-icons/fa-solid/Clipboard',
    'Chart.js': 'Chart.js',
    'react': 'react',
    'react-dom': 'react-dom',
  },
out
├── Chart.js.js
├── chunk-5732a1e2.js
├── chunk-7be8ab94.js
├── react-dom.js
├── react.js
├── styled-components.js
└── styled-icons
    └── fa-solid
        └── Clipboard.js
@dy

This comment has been minimized.

Copy link

commented Jun 30, 2019

@FredKSchott

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

hey all! I'm back from vacation and really excited about this feature. I think there are two paths to go down that would make sense here:

  1. Quick fix: For packages whitelisted in webDependences, support fallbacks for when the "module" entrypoint is not found. Basically, check for "module", then "browser", then finally "main" with a backwards-compat CJS. This will get rid of some of the verbosity problems around our current CJS backwards-compat story.
  2. Larger fix: I think we want to move forward (carefully) with support for auto-detecting/analyzing imports, which could also help with this issue.

What do people thing? I'm still catching up on issues/PRs, so apologies if I'm missing anything that's already been said outside of this thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.