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

now would be a good time to start testing with master

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

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

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.)

ah … good idea - a “release candidate”

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

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.

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

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

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

applied this change to master

^ I like this

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

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

But I might not get to it until tomorrow

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

great to hear

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

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

Sounds common enough to warrant a solution

Would also need to be loaded before every namespace

I can predict some people having problems achieving this last part

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

and one flag to disable the whole behavior if desired

Sounds good to me

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

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

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

tests pass, smoke testing now

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

bowser required by material-ui doesn’t work

stylis required by styled-components doesn’t work

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

and the list goes on 😛

leaflet.js required by mapbox.js added today

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

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

it won’t work for "module" entries

nah main is fine

eg. for stylis GCC complains about an invalid CommonJS wrapper

trying styled-components

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

for stylis

yes. ok I get the same error

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

looks related to AMD

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

works if I don’t set setTransformAMDToCJSModules

but it won’t convert anything then?

dunno

in my tests it just leaves the file untouched then

it does convert

I think

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

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

yeah

I don’t really care about AMD tbh

it’s dead in the water anyway

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

ah no wait

it doesn’t work if I read that correctly

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) {

oh no it does … nevermind

no … it doesnt dammit

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

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

that looks more related to weird dynamic stuff in JS

than module processing

no the wrapper just prevents it from rewriting the code properly

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

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

eg. styled-components or material-ui

we never promised to consume every module out there

that problem was there before CLJS-2279

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

yes I’m not really concerned about everything working

that’s just a losing game

https://dev.clojure.org/jira/browse/CLJS-2280

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

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

so I would hold off on the release?

sure, I want to do 2280 as well

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

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

fails early

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

can look at that after all this other stuff

@dnolen patch attached to https://dev.clojure.org/jira/browse/CLJS-2279

3-line diff

rest is tests

Nice

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

just setTransformAMDToCJSModules to true

https://dev.clojure.org/jira/secure/attachment/17124/CLJS-2279.patch

in addition to the (.setProcessCommonJSModules true)

that’s not realistic

But I mean how would a user do this?

IMO

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

@anmonteiro but the rewritten stylis file does not work

so its better to fail than pretending it worked

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

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

it’ll make other modules fail

that work otherwise

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

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

I won’t generalize without trying out some more

but why would you set the AMD transformation to true

if they don’t provide an AMD module

because it should fail the packages that won’t work

it doesn’t rewrite if there is no wrapper

Setting AMD to true by default is not going to happen

Given that what is under discussion?

PLEASE take one minute and consider what I am saying

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

I am considering, but you need to be more clear

if you DO SET IT it fails at compile time

which is the better option of the two

Then the problem is stylis u already said that

U -> I

If we cannot detect AMD not our problem

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

So what is under discussion?

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

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

I'm not convinced

The answer is just no for now

and your argument for that is?

Already stated

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

and 0 packages where setting it actually hurts

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

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

gotcha

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

that would work if you unset it

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

yes

doesn’t convince me immediately

heck make it a config option to unset it

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

so far I haven’t found one

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

The sky is not falling, later we can choose

ok, I think this thing is ready to go

I just tested https://twitter.com/ajpierce/status/890750681342242817

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

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

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

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

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

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

passing here

trying to build now