google / incremental-dom

An in-place DOM diffing library
http://google.github.io/incremental-dom/
Apache License 2.0
3.54k stars 180 forks source link

Source files not valid modules on the web #324

Closed justinfagnani closed 7 years ago

justinfagnani commented 7 years ago

Since native ES6 modules are finally arrive in browser, I decided to port an app that uses incremental-dom to pure ES6 modules and only modules for code loading.

incremental-dom is pretty close to working, but I ran into two problems:

  1. Module specifiers do not include file extensions, so I can't serve idom source from a static file server.

    Changing imports and exports from

    export { symbols } from './src/symbols';

    to

    export { symbols } from './src/symbols.js';

    Fixes this.

  2. There are a lot node-specific properties in the source. I had to comment-out many instances of:

    if (process.env.NODE_ENV !== 'production') {
      ...
    }

And a very minor point is that index.js is an odd name to import, as it's not really an index module (I know it's fashion in the node world). My imports look like:

import * as idom from '../../incremental-dom/index.js';

The good news is that once the two fixes are made, I can successfully load my app in Safari Technology Preview!

sparhami commented 7 years ago

Module specifiers do not include file extensions, so I can't serve idom source from a static file server.

As long as we can build with the imports using .js, that sounds totally fine to me.

There are a lot node-specific properties in the source.

I'm not sure how many people are building from source using node directly, probably not many. I think making that check just be something like if (DEBUG) { ... }, then have our common js build target replace that with process.env.NODE_ENV !== 'production' it might work. I think you would still need need to define window.DEBUG before loading the script, but wouldn't be too bad. I'm not sure if there is any other convention people use for debug flags, but totally open to whatever.

justinfagnani commented 7 years ago

Oh, I want to clarify. I'm not building from source. I'm trying to use the source directly out of node_modules/incremental-dom/.

I would like the module source in the npm package to be directly usable in a browser without any build step.

A note on how I'm doing this:

My import of idom like:

import * as idom from '../../incremental-dom/index.js';

is me importing from my-project/src/foo.js. I'm using a devserver which treats my project as a sibling to all other node_modules, so that ../incremental-dom from my project root reaches into node_modules/incremental-dom/. I've installed everything flat into node_modules with yarn install --flat.

sparhami commented 7 years ago

Oh, I want to clarify. I'm not building from source. I'm trying to use the source directly out of node_modules/incremental-dom/.

To clarify, the change is fine, but it need to make sure our build works. It looks like our existing version of Closure Compiler (used for type checking and verifying it is valid closure js) doesn't like importing with the extension and a later version has an issue with the type definition for getRootNode. I'll look into fixing it.

justinfagnani commented 7 years ago

@sparhami I'm super excited that you landed #327!

but... :) it only fixes the module specifier part of this issue. There are still Node-specific properties being access that cause errors in browsers. Should I open a separate issue for that?

cedeber commented 7 years ago

As @justinfagnani just said, some files still have some process.env.NODE_ENV tests.

sparhami commented 7 years ago

but... :) it only fixes the module specifier part of this issue. There are still Node-specific properties being access that cause errors in browsers. Should I open a separate issue for that?

Oops, missed that part. Will take another look.