Fork me on GitHub
#cljs-dev
<
2017-07-28
>
dnolen13:07:23

I’ve deprioritized the warning on ^:private usage - we have no more critical tickets

dnolen13:07:36

now would be a good time to start testing with master

dnolen13:07:03

@anmonteiro I think CLJS-2279 would be worth addressing @thheller’s investigation here seems like the path to pursue

dnolen13:07:21

unless the overall sentiment is that we should get out what we have now, and address that in a future release

mfikes14:07:50

There might be value in doing a pre-release (like https://github.com/clojure/clojurescript/releases/tag/r1.7.189) making it easier for people to try the NPM stuff. (My guess is that corner cases are going to be found, some addressable, some not.)

dnolen14:07:18

ah … good idea - a “release candidate”

dnolen14:07:35

and then we have time to fold in any issues that crop later, or further refinements

mfikes14:07:56

Yeah, to us it seem trivial to build ClojureScript. Perhaps an order magnitude more people can try it if it is just a project.clj tweak.

dnolen14:07:24

ok, so then I’ll just cut a release today and put together a short blog post about it - later I can post about :global-exports

dnolen14:07:37

will hold off till later in the afternoon - so there’s time to get some more feedback from people on the West Coast like António

dnolen14:07:00

I’m thinking we should always disable non standard JSDoc warnings for the JS modules types - these things are never going to be Closure stuff

dnolen15:07:31

applied this change to master

anmonteiro15:07:14

^ I like this

anmonteiro15:07:07

It's that kind of thing that I keep doing all the time but don't feel enough pain to automate

anmonteiro15:07:26

I wanna try Thomas's approach too, it sounds like it would solve the ES6 problem if it works

anmonteiro15:07:46

But I might not get to it until tomorrow

anmonteiro15:07:34

@dnolen FWIW I've been testing Lumo with master every other day and everything looks good

dnolen15:07:20

great to hear

anmonteiro15:07:48

There's maybe 1 self-host issue that we know about and it's in JIRA related to macro inference but doesn't seem critical to me

dnolen15:07:29

I’m also wondering if we shouldn’t just do the process.env dance for users

anmonteiro15:07:58

Sounds common enough to warrant a solution

anmonteiro15:07:14

Would also need to be loaded before every namespace

anmonteiro15:07:33

I can predict some people having problems achieving this last part

dnolen15:07:49

yeah I’m thinking we just provide the namespace, it’s always preloaded, and we set it automatically for compilation levels

dnolen15:07:59

and one flag to disable the whole behavior if desired

anmonteiro15:07:02

Sounds good to me

anmonteiro16:07:37

@dnolen experimenting with @thheller ‘s suggestions right now for processing es6 modules

anmonteiro16:07:01

preliminary results are good, but I’m making sure by re-running tests again

thheller16:07:56

I’ve been messing with my own implementation for importing npm sources

anmonteiro16:07:17

tests pass, smoke testing now

thheller16:07:22

there are a bunch of packages that don’t work in my testing, didn’t test if cljs.closure works for them

thheller16:07:33

bowser required by material-ui doesn’t work

thheller16:07:56

stylis required by styled-components doesn’t work

thheller16:07:21

@atlaskit/navigation doesn’t work when using package.json module

thheller16:07:34

and the list goes on 😛

thheller16:07:52

leaflet.js required by mapbox.js added today

thheller16:07:43

@anmonteiro can you try any of those packages to see if it works with whats in CLJS?

thheller16:07:57

I don’t use any of it so it just might be bugs in my code

anmonteiro16:07:09

it won’t work for "module" entries

thheller16:07:17

nah main is fine

thheller16:07:40

eg. for stylis GCC complains about an invalid CommonJS wrapper

anmonteiro17:07:42

trying styled-components

anmonteiro17:07:55

also got ERROR: NON_TOP_LEVEL_STATEMENT_DEFINE. The define function must be called as a top-level statement

thheller17:07:14

yes. ok I get the same error

thheller17:07:54

I though that might have been and issue due to always enabling all rewrites but I guess not

anmonteiro17:07:18

looks related to AMD

thheller17:07:36

yeah they have a non-standard wrapper which closure simply doesn’t recognize

anmonteiro17:07:52

works if I don’t set setTransformAMDToCJSModules

thheller17:07:01

but it won’t convert anything then?

thheller17:07:16

in my tests it just leaves the file untouched then

anmonteiro17:07:21

it does convert

anmonteiro17:07:40

goog.provide("module$Users$anmonteiro$Documents$github$clojurescript$node_modules$stylis$stylis")

thheller17:07:56

hmm ok then it isn’t safe to always enable that I guess 🙂

anmonteiro17:07:21

I don’t really care about AMD tbh

anmonteiro17:07:26

it’s dead in the water anyway

anmonteiro17:07:40

as long as we get ES6 / CommonJS I’m as happy as I can be

thheller17:07:22

it doesn’t work if I read that correctly

thheller17:07:03

goog.provide("module$node_modules$stylis$stylis");
var module$node_modules$stylis$stylis = {};
(function(factory) {
  typeof module$node_modules$stylis$stylis === "object" && typeof module !== "undefined" ? module["exports"] = factory(null) : typeof define === "function" && define["amd"] ? define(factory(null)) : window["stylis"] = factory(null);
})(function factory(options) {

thheller17:07:37

oh no it does … nevermind

thheller17:07:50

no … it doesnt dammit

thheller17:07:13

that should not work module["exports"] = factory(null)

thheller17:07:49

and module is undefined so it will window["stylis"] = factory(null)

anmonteiro17:07:22

that looks more related to weird dynamic stuff in JS

anmonteiro17:07:27

than module processing

thheller17:07:05

no the wrapper just prevents it from rewriting the code properly

anmonteiro17:07:39

well exactly, but most NPM modules won’t have that shit

thheller17:07:56

that doesn’t matter if some of the most popular packages use it

thheller17:07:07

eg. styled-components or material-ui

anmonteiro17:07:15

we never promised to consume every module out there

anmonteiro17:07:26

that problem was there before CLJS-2279

anmonteiro17:07:50

I’m just focused on being able to support random NPM modules without having to bend over backwards trying to figure out if they’re ES6 or CommonJS

dnolen17:07:10

yes I’m not really concerned about everything working

dnolen17:07:13

that’s just a losing game

dnolen17:07:26

things will work if people want them to work, not our problem

anmonteiro17:07:18

@dnolen looks like we might be able to solve CLJS-2279 cleanly and easily

anmonteiro17:07:22

so I would hold off on the release?

dnolen17:07:18

sure, I want to do 2280 as well

thheller17:07:29

FWIW I think its better to fail rewriting than rewriting and not working at runtime

thheller17:07:50

eg. ERROR: NON_TOP_LEVEL_STATEMENT_DEFINE. The define function must be called as a top-level statement this error is “good”

thheller17:07:00

fails early

dnolen17:07:56

I don’t have a problem with failing if Closure reports an error

dnolen17:07:35

can look at that after all this other stuff

anmonteiro17:07:55

rest is tests

dnolen17:07:31

@thheller do you have a minimal case so I can look at failing early?

thheller17:07:10

just setTransformAMDToCJSModules to true

thheller17:07:37

in addition to the (.setProcessCommonJSModules true)

anmonteiro17:07:50

that’s not realistic

dnolen17:07:52

But I mean how would a user do this?

anmonteiro17:07:10

we should probably look at a module that can’t be processed

thheller17:07:11

@anmonteiro but the rewritten stylis file does not work

thheller17:07:30

so its better to fail than pretending it worked

anmonteiro17:07:38

but I don’t think we should setProcessAMDToCJSModules to true in every case

dnolen17:07:39

I only care about something a user can do, like a minimal case

anmonteiro17:07:44

it’ll make other modules fail

anmonteiro17:07:48

that work otherwise

thheller17:07:57

@anmonteiro are you sure that those packages work? in case of stylis it does not work at runtime

thheller17:07:16

transform works if you disabled the amd conversion but the runtime doesn’t

anmonteiro17:07:21

I won’t generalize without trying out some more

anmonteiro17:07:49

but why would you set the AMD transformation to true

anmonteiro17:07:58

if they don’t provide an AMD module

thheller17:07:01

because it should fail the packages that won’t work

thheller17:07:28

it doesn’t rewrite if there is no wrapper

dnolen17:07:53

Setting AMD to true by default is not going to happen

dnolen17:07:11

Given that what is under discussion?

thheller17:07:28

PLEASE take one minute and consider what I am saying

thheller17:07:48

if you DO NOT SET IT and rewrite node_modules/stylis it pretends to work but fails at runtime

dnolen17:07:50

I am considering, but you need to be more clear

thheller17:07:57

if you DO SET IT it fails at compile time

thheller17:07:03

which is the better option of the two

dnolen17:07:11

Then the problem is stylis u already said that

dnolen17:07:53

If we cannot detect AMD not our problem

thheller17:07:59

I did not run into issues with always enabling the rewrite. only for packages that would otherwise not work anyways

dnolen17:07:04

So what is under discussion?

thheller17:07:17

> Setting AMD to true by default is not going to happen

thheller17:07:29

just always set it .. is what I am saying …

dnolen17:07:32

I'm not convinced

dnolen17:07:42

The answer is just no for now

thheller17:07:02

and your argument for that is?

dnolen17:07:25

Already stated

thheller17:07:37

I can point you to at least 4 packages where setting it is beneficial

thheller17:07:48

and 0 packages where setting it actually hurts

anmonteiro17:07:03

the question is really, do those packages provide AMD modules?

thheller17:07:34

@anmonteiro that is not the question .. if they don’t contain AMD wrapper closure will do nothing

thheller17:07:57

so the question is: does it hurt setting it? I have not run into a package where it does

thheller17:07:06

that would work if you unset it

anmonteiro17:07:07

so the advantage of setting AMD processing to true is just about failing early?

anmonteiro17:07:45

doesn’t convince me immediately

thheller17:07:49

heck make it a config option to unset it

thheller17:07:12

show me a package where it is a problem and you’ll convince me

thheller17:07:17

so far I haven’t found one

dnolen17:07:03

You're checks are going to compare to everyone checking a released thing

dnolen17:07:19

The sky is not falling, later we can choose

dnolen20:07:17

ok, I think this thing is ready to go

dnolen20:07:37

and it works but way simpler now with the process lib shimming

dnolen20:07:07

basically the only thing a users need to do is provide externs

dnolen20:07:29

and they wouldn’t even need to do that if we got a patch into React

dnolen20:07:18

@mfikes hrm self parity tests seem to be failing here?

dnolen20:07:08

nvm, I think I needed to clean and re-bootstrap

mfikes20:07:14

Cool. Self-parity was passing in my CI with the last commit https://github.com/mfikes/clojurescript/commits/master

dnolen21:07:50

passing here

dnolen21:07:54

trying to build now