ember-decorators / argument

Decorators for Component and Object arguments in Ember
MIT License
30 stars 18 forks source link

[feature-request] Ignore attributes by prefix / for ember-ast-hot-load #89

Closed lifeart closed 5 years ago

lifeart commented 5 years ago

During Handlebars AST compile time I add few props for component invocation:

hotReloadCUSTOMhlContext, hotReloadCUSTOMName, hotReloadCUSTOMhlProperty, hotReloadCUSTOMHasParams, hotReloadCUSTOMHasHash,

is it possible to whitelist it by prefix? hotReloadCUSTOM or allow users to add it into addon config

alexlafroscia commented 5 years ago

Sounds reasonable to me! I think this would be a great thing to add support for.

buschtoens commented 5 years ago

Maybe a regex and / or predicate function would be more flexible?

alexlafroscia commented 5 years ago

@buschtoens do you have thoughts on what the API to whitelist additional attributes might look like with those suggestions?

buschtoens commented 5 years ago

Nothing concrete, no. I guess that we would want this setting to live in the config/environment.js, another file in config/ or ember-cli-build.js.

'@ember-decorators/argument': {
  whitelist: [
    /^hotReloadCUSTOM/,
    key => key.startsWith('hotReloadCUSTOM')
  ]
}

I think you can't easily have non-primitive values in config/environment.js though. So maybe some extra file would be required.

lifeart commented 5 years ago

or it can be array of plain strings with startsWith logic. whitelist: ['bs-', 'hotReloadCUSTOM', 'liquid-fire']

buschtoens commented 5 years ago

@lifeart sure, that's what it would be for prefixes only.

lifeart commented 5 years ago

prefixes + full names. or whitelist: { startsWith: ['-bs'], includes: ['-dummy'], endsWith: ['-form'] }

alexlafroscia commented 5 years ago

I think a regex approach is probably the easiest way to go — very flexible and easy to express exact match, starting with, ending with, etc.

alexlafroscia commented 5 years ago

Ugh... So, neither functions nor regular expressions actually are translated through the config/environment.js file correctly. Regular expressions come through as {} and a function came through as null.

I'm going to approach this by allowing either of these:

// array of exact matches
attributeWhitelist: ['foo']

or

// define how you want to match
attributeWhitelist: {
  startsWith: [],
  includes: [],
  endsWith: []
  matches: []
}

If we can figure out a way to handle regular expressions, I would love to simplify that, but that's the plan for now.

lifeart commented 5 years ago

regexp in config can be in string form, and we can do new RegExp(string); as mentioned in https://github.com/ember-decorators/argument/pull/91/files#diff-164d647270288733c95f5bd657121df7R23

alexlafroscia commented 5 years ago

Yup, that's basically what my PR does! Each of the different keys in the object-style whitelist creates the RegExp a different way.

https://github.com/ember-decorators/argument/blob/abfe66c80f6d04e8dea4417ea6c64851bae1eb9b/addon/-private/config.js#L22-L30

lifeart commented 5 years ago

should we have few params or just allow users to specify regexp string body in config? attributeWhitelist: ['^$fooBar', '$-form$', 'exact']

alexlafroscia commented 5 years ago

Hmm, yeah that might make more sense. My only worry would be that it’s a little hard to explain what’s required — saying “put a regex in the string” will likely lead to some people accidentally putting a regex in there, which will break. On the other hand, simpler code and a cleaner API... Not sure which I like better, would love other opinions!

buschtoens commented 5 years ago

Is this addon still a no-op in production builds? In this case, we might want an extra file instead, which would allow us to import it as is and even strip it from production builds.

lifeart commented 5 years ago

@alexlafroscia it can be kinda "hidden" feature, noted in *. By default we can specify it should be full names: attributeWhitelist: ['someProperty', 'isEnabled', 'astHOTLOAD'] and only power users may know it may be an stringed regexp )

lifeart commented 5 years ago

also, it can be path-like item. attributeWhitelist: ['bsDropdown.readOnly'] where bsDropdown === component.constructor.name || component._debugContainerKey || component.toString()

alexlafroscia commented 5 years ago

Is this addon still a no-op in production builds?

Yup, even more so in 1.0.0 where all of the behavior is stripped in production. The pre-1.0.0 version key a small amount of runtime behavior in production builds but stripped most of it. That behavior is built into ember 3.4+ now, so we're aligning with that.

In this case, we might want an extra file instead, which would allow us to import it as is and even strip it from production builds.

I'm not sure what you mean -- could you explain?

buschtoens commented 5 years ago

Yes, sure! I mean something like config/ember-intl.js. While in this case the file is required by Node, we could easily make it part of the build pipeline. Alternatively, we could do it like liquid-fire's transitions.js and put the file at the root of the app.

alexlafroscia commented 5 years ago

Interesting -- I think I like the idea of doing something like config/ember-decorators/arguments.js (or something else in the config directory) better. I personally don't love putting a config file in app.